ExpectedSystemExit improvement in multi-threads environment #46
Conversation
| private Integer statusOfFirstExitCall = null; | ||
|
|
||
| public NoExitSecurityManager(SecurityManager originalSecurityManager) { | ||
| this(originalSecurityManager, new EmptySynchLatch()); |
There was a problem hiding this comment.
Please delete this constructor because we don't need it anymore. It is only used by the test and you can use new NoExitSecurityManager(..., 0) instead.
There was a problem hiding this comment.
I just saw that this was the case before your last commit. I suggest to revert your last commit because it does not improve the code in a way that I want. It adds another layer of abstraction by using a new type. I prefer simple code :-)
There was a problem hiding this comment.
Nevertheless you can delete the old constructor.
There was a problem hiding this comment.
@stefanbirkner good point! agreed.
Conflicts: src/main/java/org/junit/contrib/java/lang/system/internal/NoExitSecurityManager.java
|
Implemented your notes. Fixed FindBug warning. |
| return statusOfFirstExitCall != null; | ||
| public boolean isCheckExitCalled() throws InterruptedException { | ||
| boolean wasCountDown = synchLatch.await(timeout, TimeUnit.MILLISECONDS); | ||
| return statusOfFirstExitCall != null && wasCountDown; |
There was a problem hiding this comment.
You don't have to check statusOfFirstExitCall anymore. You can remove the second line and immediately return the result of line 36: return synchLatch.await(...). By the way could you rename syncLatch to exitLatch.
There was a problem hiding this comment.
@stefanbirkner agreed. Also it's possible to change type of statusOfFirstExitCall from Integer to simply int and remove if statement at all here:
@Override
public void checkExit(int status) {
if (statusOfFirstExitCall == null)
statusOfFirstExitCall = status;
exitLatch.countDown();
throw new CheckExitCalled(status);
}
WDYT?
| handleSystemExitWithStatus(securityManager.getStatusOfFirstCheckExitCall()); | ||
| else | ||
| handleMissingSystemExit(); | ||
| } catch (InterruptedException e) { |
There was a problem hiding this comment.
Don't catch that exception. By rethrowing it, the developer gets the full stacktrace of that exception which can be valuable. Additionally it is less code on our side.
|
We are getting closer to a final version of the pull request. Thank you for your patience. |
| throw new CheckExitCalled(status); | ||
| } | ||
|
|
||
| public boolean isCheckExitCalled() { | ||
| return statusOfFirstExitCall != null; | ||
| public boolean isCheckExitCalled() throws InterruptedException { |
There was a problem hiding this comment.
Could you add the timeout as a parameter to the isCheckExitCalled method instead of using a field in that class.
|
@stefanbirkner you have to supprese Firebugs check for intentional ignoring return value in line |
|
Here is information how to do this |
Implementation of #44 fix