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

196 - modifications to allow the ThreadedSocketAcceptor to be restarted after a calling acceptor.stop #351

Conversation

huwmjenkins
Copy link
Contributor

@huwmjenkins huwmjenkins commented Sep 21, 2015

#196

There were numerous changes required for this issue.

The first was that Acceptor.Start() only calls the StartAcceptingConnections method if the isStarted_ flag is false. However, on calling Acceptor.Stop() it does nothing with this flag. So the first change was to have Acceptor.Stop() setting isStarted_ to false.

However, the SocketReactor.Start method which is called by the StartAcceptingConnections method only actually runs if the ReactorState is set to Running but the state_ field is only set on construction. So I've changed the code so that on construction of the reactor state_ is set to SHUTDOWN_COMPLETED and only on calling start does is get set to RUNNING.

The next issue was that after restarting the connection and trying to logon the SocketReader would complain that "Multiple logons/connections for this asession are not allowed". This was was related to the non implementation of WaitForLogout. So, I've implemented this method.

However, this didn't finally resolve the issue either as there were some threading issues going on:

The first was in the ClientHandlerThread in that it could be disposed but the Log method would be called from the reactor so I've just added a null check there which resolved that issue.

The next issue was that the SocketReactor.Run method uses TcpListener.AcceptTcpClient which is a blocking method which meant that if no one was trying to connect then the While{} loop would never break and so the connections would never shutdown. I changed this so that is uses a while loop and a Wait Handle on TcpListener.Pending(). (However, it would probably be better to change this to using the asynchronous BeginAcceptTcpClient/EndAcceptTcpClient methods but I have to work on something else just yet but I may try implementing it with these methods instead.)

The final threading issue was that the SocketReactor.Run method first action was to call TcpListener.Start. However, it is possible for stop to be called before the start method has finished which would then leave the socket reactor in a pending stop state. So I simply moved this to the SocketReactor.Start method before the thread is called.

I also made one slight change in the ordering of the Acceptor.stop method it will now try to logout all sessions before trying to stop the connections. This made the behaviour more consistent in terms of logout messages being sent before the connections were dropped. I think it makes more sense to try to logout before disconnection. Also as the Logout method disables the sessions even if a counterparty tries to connect, the session won't be accepted as it is disabled.

@CLAassistant
Copy link

CLAassistant commented Apr 29, 2016

CLA assistant check
All committers have signed the CLA.

@huwmjenkins huwmjenkins closed this Jul 5, 2017
@huwmjenkins huwmjenkins reopened this Jul 5, 2017
gbirchmeier and others added 20 commits March 9, 2020 11:32
…ettings

Add the ability to configure socket send and receive timeouts
clientId can clash if there's more than one Acceptor endpoint listening on different IP or Port but trying to write to the same debug log file as all starting at 0.  

Using ticks to hopefully keep unique in between each client logon attempt.
Use Ticks in place of clientID to avoid debug log file name clash (resubmit of connamara#383)
I accidentally left this in 544c87e (connamara#582)
This bit actually triggers a known other bug.
Since the test is redundant anyway, delete it.
Bugfix: SocketAcceptPort not fully obeyed (rebase of connamara#410)
@gbirchmeier
Copy link
Member

@huwmjenkins, I'm tackling the PR backlog on this project.

I like your PR, but there are some conflicts when I tried to upmerge it. Would you be willing to upmerge it one more time? I will be ready to review and merge as soon as you can find the time.

I apologize for the long lifetime of this PR. I'm wrangling this project back into order, and working hard to clear up the backlog. Thanks.

…com:huwmjenkins/quickfixn into Issue-196-AcceptorStopThenRestartDoesntWork
@huwmjenkins
Copy link
Contributor Author

I've brought the PR up-to-date but I haven't looked over it too extensively, the tests do pass but as it's been three years I'm not as familiar with it as once I was. Especially when it comes to .net standard as opposed to the full .net framework. I will try and schedule a bit more time to look at it a little more thoroughly but if you spot anything amiss let me know.

@gbirchmeier
Copy link
Member

Thanks, @huwmjenkins. We will review and try to wrap this up.

@gbirchmeier
Copy link
Member

@mgatny pulled and pushed into #586, which seems to have gotten around whatever Github weirdness is causing this diff to be so gross. So I'll close this one and we'll proceed off of that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.