Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not all intervals are transferred at shutdown #230

Open
nspruit opened this issue Jan 13, 2016 · 16 comments
Open

Not all intervals are transferred at shutdown #230

nspruit opened this issue Jan 13, 2016 · 16 comments
Assignees
Labels

Comments

@nspruit
Copy link
Collaborator

nspruit commented Jan 13, 2016

It seems like there is a bug in transferring the closed intervals to the server when WD is shut down. Any intervals that are still open are closed before shutting down WD and the remaining intervals are transferred to the server. However, not all of these intervals are actually tranferred.

For instance, see the following log:

Jan 13, 2016 2:52:17 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Starting up WatchDogLogger...
Jan 13, 2016 2:52:17 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Starting WatchDog ...
Jan 13, 2016 2:52:18 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEOpenInterval@87060c0 IDE_OPEN
Jan 13, 2016 2:52:18 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.UserActiveInterval@42eaa429 USER_ACTIVE
Jan 13, 2016 2:52:18 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.PerspectiveInterval@669bb57 PERSPECTIVE
Jan 13, 2016 2:52:18 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEActiveInterval@60ccf498 IDE_ACTIVE
Jan 13, 2016 2:52:29 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.DebugInterval@359d0007 DEBUG
Jan 13, 2016 2:52:30 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.ReadingInterval@137b9be1 READING
Jan 13, 2016 2:52:32 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.UserActiveInterval@42eaa429 USER_ACTIVE
Jan 13, 2016 2:52:32 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.ReadingInterval@137b9be1 READING
Jan 13, 2016 2:52:32 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEOpenInterval@87060c0 IDE_OPEN
Jan 13, 2016 2:52:33 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.PerspectiveInterval@669bb57 PERSPECTIVE
Jan 13, 2016 2:52:33 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEActiveInterval@60ccf498 IDE_ACTIVE
Jan 13, 2016 2:52:33 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.DebugInterval@359d0007 DEBUG
Jan 13, 2016 2:52:33 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Data length: 0.2412109375 kB
Jan 13, 2016 2:52:35 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Shutting down Plugin...

As you can see, 6 intervals are created and they are also closed before shutting down. However, only two intervals are received at the server that I run locally: IDE_OPEN and USER_ACTIVE. Therefore, the remaining four intervals seem to be lost.

P.S. Is this issues related to #122 @Inventitech?

@Inventitech
Copy link
Contributor

Hm, very strange, this is new to me. Could you debug a bit into this, please?

@nspruit
Copy link
Collaborator Author

nspruit commented Jan 13, 2016

Sure, will do

@Inventitech
Copy link
Contributor

👍

@nspruit
Copy link
Collaborator Author

nspruit commented Jan 13, 2016

I have some more info after debugging for a while. I've added some print statements, resulting in the following log:

Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=256m; support was removed in 8.0
Jan 13, 2016 4:41:10 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Starting up WatchDogLogger...
Jan 13, 2016 4:41:10 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Starting WatchDog ...
Jan 13, 2016 4:41:11 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEOpenInterval@1925be93 IDE_OPEN
Jan 13, 2016 4:41:11 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.UserActiveInterval@2ec3c213 USER_ACTIVE
Jan 13, 2016 4:41:11 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.PerspectiveInterval@b92cdf9 PERSPECTIVE
Jan 13, 2016 4:41:11 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEActiveInterval@464aff63 IDE_ACTIVE
Jan 13, 2016 4:41:15 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.DebugInterval@478dc028 DEBUG
Jan 13, 2016 4:41:16 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Created interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.ReadingInterval@74d2525f READING
Set before: 0
Set After: 1; Added?: true
Set before: 134
Set After: 135; Added?: true
Jan 13, 2016 4:41:19 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.UserActiveInterval@2ec3c213 USER_ACTIVE
Set before: 1
Set After: 1; Added?: false
Set before: 135
Set After: 135; Added?: false
Jan 13, 2016 4:41:19 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.ReadingInterval@74d2525f READING
Intervals to be closed: [nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEOpenInterval@1925be93, nl.tudelft.watchdog.core.logic.interval.intervaltypes.PerspectiveInterval@b92cdf9, nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEActiveInterval@464aff63, nl.tudelft.watchdog.core.logic.interval.intervaltypes.DebugInterval@478dc028]
Set before: 1
Set After: 2; Added?: true
Set before: 135
Set After: 136; Added?: true
Jan 13, 2016 4:41:19 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEOpenInterval@1925be93 IDE_OPEN
Set before: 2
Set After: 2; Added?: false
Set before: 136
Set After: 136; Added?: false
Jan 13, 2016 4:41:20 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.PerspectiveInterval@b92cdf9 PERSPECTIVE
Set before: 2
Set After: 2; Added?: false
Set before: 136
Set After: 136; Added?: false
Jan 13, 2016 4:41:20 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEActiveInterval@464aff63 IDE_ACTIVE
Set before: 2
Set After: 2; Added?: false
Set before: 136
Set After: 136; Added?: false
Jan 13, 2016 4:41:20 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: closed interval nl.tudelft.watchdog.core.logic.interval.intervaltypes.DebugInterval@478dc028 DEBUG
To transfer: [nl.tudelft.watchdog.core.logic.interval.intervaltypes.UserActiveInterval@2ec3c213, nl.tudelft.watchdog.core.logic.interval.intervaltypes.IDEOpenInterval@1925be93]
Jan 13, 2016 4:41:20 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Data length: 0.2412109375 kB
Jan 13, 2016 4:41:22 PM nl.tudelft.watchdog.core.util.WatchDogLogger logInfo
INFO: Shutting down Plugin...

So, as you can see, some intervals are not added to the set of intervals to persist. In particular, when shutting down, four intervals still need to be closed. However, only one of those intervals is actually added to the set of intervals to persist. Therefore, at the end only two intervals are transferred to the server and the other intervals are simply lost.

When an interval is not correctly added, the return value of set.add() is false. Therefore, it seems like this issue is caused by a defect in the equality checks of the intervals.

I'll look further into this bug tomorrow morning and try to fix it

@nspruit
Copy link
Collaborator Author

nspruit commented Jan 13, 2016

In addition, in the log shown above you can see that the bug does not only appear before shutting down. The ReadingInterval is also lost due to this bug, which is already closed before shutdown. This means that this seems to be a quite severe bug, as we are losing quite some data in this way.

nspruit pushed a commit to nspruit/watchdog that referenced this issue Jan 14, 2016
nspruit pushed a commit to nspruit/watchdog that referenced this issue Jan 19, 2016
nspruit pushed a commit to nspruit/watchdog that referenced this issue Jan 20, 2016
nspruit pushed a commit to nspruit/watchdog that referenced this issue Jan 21, 2016
@Inventitech
Copy link
Contributor

Thanks a lot for the bug fix.

@Inventitech Inventitech added this to the WatchDog 1.7 milestone Jan 25, 2016
@Inventitech Inventitech reopened this Apr 14, 2016
Inventitech added a commit that referenced this issue Apr 14, 2016
Adds an EqualsVerifier test to detect possible problems in our equals implementation. Purposefully
breaks the build, as the contract is violated (test fails).

Refs #230
@Inventitech
Copy link
Contributor

Sorry for reopening this, but see #243, which will fail in a few minutes :-)

Could you take care of fixing it?

@nspruit
Copy link
Collaborator Author

nspruit commented Apr 14, 2016

#243 indeed fails, but not because of the problem described (at least not in this build).

However, I've checked out the branch and the test does actually fail. I'll see if I can easily fix this

Inventitech added a commit that referenced this issue Apr 14, 2016
@Inventitech
Copy link
Contributor

OK, I fixed the fail and now the build actually fails for the right reason (double-fail) ;-)

@nspruit
Copy link
Collaborator Author

nspruit commented Apr 14, 2016

👍 I've now fixed that hashCode error by adding a hashCode method to IntervalBase. Unfortunately this results in new errors...

The question I have now is: can an instance of IntervalBase be equal to an instance of WatchDogTransferable or vice versa?

@nspruit
Copy link
Collaborator Author

nspruit commented Apr 14, 2016

BTW, as WatchDogTransferable is an abstract class there can never be another instance of it except for instances of IntervalBase or EventBase. These two options both override the equals method of WDT, which eliminates the situation described in my question above (as they can never be equal) or am I wrong?

@nspruit
Copy link
Collaborator Author

nspruit commented Apr 15, 2016

After adding several different new equals and hashCode methods to WatchDogTransferable and IntervalBase as well as canEqual methods as described here, errors still appear when running the test. Furthermore, I still don't really get what problem/situation we are actually trying to fix here...

@Inventitech
Copy link
Contributor

Niels, can you refer me to the commits you speak about?

@nspruit
Copy link
Collaborator Author

nspruit commented Apr 18, 2016

I haven't committed anything as I never got the test to pass, so I'm not referring to any commits here

@Inventitech
Copy link
Contributor

The problem is that we are violating the equals contract -- at least according to the EqualsVerifier. As you also said, I think this is more of a theoretical problem at this point. However, I would like to understand precisely why it complains and will investigate this later myself.

Removing the WD-2.0 tag for now, so we can release.

@Inventitech Inventitech removed this from the WatchDog 2.0 milestone Apr 19, 2016
@nspruit
Copy link
Collaborator Author

nspruit commented Apr 20, 2016

Exactly, practically every change I made based on the given error messages did not change the test results, so it seems to be a theoretical issue indeed. However, I don't know if it is even possible to fix this in practice, but we'll see.

I started with a missing hashcode error so I generated a hashCode and new equals method for IntervalBase. Then a new error appeared. Trying to fix that one (and subsequent errors) using the explanations here finally results in "equals not final". However, when you make the equals final, you can't use mutable fields inside it. Unfortunately, almost all field are mutable and can't be made final either. So, then you can start all over again. For me, this process of different subsequent errors just continued whatever I changed. So finally, I just gave up for now;)

@Inventitech Inventitech added this to the WatchDog 2.0.1 milestone Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants