[QFJ-813] SessionConnector:waitForLogout()'s InterruptedException handling. Created: 11/Oct/14 Updated: 18/Sep/17 |
|
Status: | Open |
Project: | QuickFIX/J |
Component/s: | Engine |
Affects Version/s: | 1.5.3 |
Fix Version/s: | 2.1-BETA |
Type: | Improvement | Priority: | Default |
Reporter: | Francesco Lo Conte | Assignee: | Unassigned |
Resolution: | Unresolved | Votes: | 0 |
Labels: | None |
Description |
When the executing thread is interrupted the waitForLogout() method catches it and prints and error to log. I would like to suggest that the exception is either thrown or at least re-asserted so that higher level application code can handle it. Also I would like to suggest that nothing is printed to the log file. This would:
Currently after the interruption is requested there is no way to know whether the engine actually stopped. I supposed a similar behavior would be desirable in all other equivalent places in the code. I would be interested to hearing your opinion about it. Many thanks. |
Comments |
Comment by Christoph John [ 13/Oct/14 ] |
So you propose to remove the logging and call Thread.currentThread().interrupt() instead? |
Comment by Francesco Lo Conte [ 13/Oct/14 ] |
Yes. Alternatively, stop catching the InterruptedException and let it be thrown by the method. The impact would be:
This would have repercussions on the Connector interface and break backward compatibility. |
Comment by Christoph John [ 16/Oct/14 ] |
OK, then we should change it to "Thread.currentThread().interrupt()". |
Comment by Francesco Lo Conte [ 17/Nov/14 ] |
SingleThreadedEventHandlingStrategy 40: I think 'isStopped' should be volatile because it is read and set by 2 different threads 50: } catch (InterruptedException e) { This is called from Mina's code so it should never happen anyway. So I think it's reasonable to throw a RuntimeException. An improved handling would be for onMessage(...) not to catch it and instead throw it so not to disrupt anything inside Mina (or the Transport layer in general) that might want to know about it. But it's probably not worth it changing the interface at this time. 79: }catch (InterruptedException e) { // ignore }No change. Can be ignored as this thread is never stopped. We don't even need a handle to it as its run() method just completes. ThreadPerSessionEventHandlingStrategy 116: } catch (final InterruptedException e) { quickfixSession.getLog().onErrorEvent(e.toString()); }This is called from Mina's code so it should never happen. So I think it's reasonable to just log an error in the session's log. 137: One suggestion: in both SingleThreadedEventHandlingStrategy and ThreadPerSessionEventHandlingStrategy there are thread that are instantiated. Then when they need to be stopped one simply calls myThread.stop(). |
Comment by Christoph John [ 15/Dec/14 ] |
Hi Francesco, thanks for your input. If it is OK with you I would put this issue to the next major release to resolve the remaining points. Thanks, |