Re-enabled acceptor sessions in finally block

[upchuck]

The Acceptor sessions should get re-enabled if socket was disconnected during logout.

When the following disconnect code runs it skips re-enable the acceptor session.
if (!hasResponder()) {
if (!ENCOUNTERED_END_OF_STREAM.equals(reason)) {
getLog().onEvent("Already disconnected: " + reason);
}
return;
}

[upchuck]

SessionConnectorTest.testConnector is failing b/c the connector state returns false for isInitiator. It seems like the connector state should return true. Maybe someone has time to investigate this further but I believe the test failure is invalid.

[chrjohn]

Thanks for the PR. I'll check the unit test.

[chrjohn]

Hi @upchuck ,
Hmm, isInitiator is correct to return false since the connector under test is an Acceptor.

Did this cause a real problem on your side? Because IMHO when disconnect() is called with a set responder, then the session will be re-enabled. When disconnect() is called (for whatever reason) a second time, then the session will not be re-enabled, however I do not see how it should get disabled in the meantime? Except someone manually called Session.logout().

Cheers,
Chris.

P.S.: But it probably does not hurt to merge the changes you did.

[upchuck]

Correct, the test uses a acceptor session. Therefore, my original assumption why the test failed is wrong. Thanks!

Yes, I have intermittent issues with acceptors sessions not able to login after dropping the connection. When I investigated, it seems the Session must be enabled for acceptor sessions to process the login message but it was not. The disconnect method doesn't seem to re-enable sessions when socket is dropped. I felt the following code was causing the problem b/c it skipped re-enable.

            if (!hasResponder()) {
                if (!ENCOUNTERED_END_OF_STREAM.equals(reason)) {
                    getLog().onEvent("Already disconnected: " + reason);
                }
                return;
            }

After reviewing the testConnector in SessionConnectorTest, I don't see why it checks session enabled is false and it seems like a invalid check. Can you review this test to see if you agree?

FYI: I made the changes in my local version and it passed my Jenkins PR with VeriFIX tests.

I'm still very interested in what the experts think about this change. Thanks!

[chrjohn]

I think you are correct and the assumption in the test is wrong. Could you please remove the lines in doubt and while you're at it convert the test into the newer JUnit style, i.e. using @Test and not extending TestCase?
Thanks in advance for your help.
Cheers,
Chris.

[chrjohn]

Hi @upchuck , I'm planning to build the 1.6.4 release this week, so if you wanted this included please update the PR. Otherwise I am trying to do the changes but I cannot say for sure if I'll have the time to do it prior to the release.
Thanks,
Chris.

[upchuck]

Sorry, but I haven't learned to use GitHub yet and I don't know how to add/update an existing PR. I won't get to this for a week or two because of deadlines at work.

[chrjohn]

No problem, I've added this to release 1.7.0 now which is going to be released later this year.
If you can't change the existing PR just submit a new one and close this.