- Notifications
You must be signed in to change notification settings - Fork81
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Attempt to fix crash after receiving a KickedOff message#1521
base:master
Are you sure you want to change the base?
Conversation
Otherwise the operations fail with an invalid handle error, which is abit less obvious when debugging.
Every AsyncSocket is guarded by its own fCritsect. All ConnectOperationsare guarded by the shared s_connectCrit.
Apparently the NetCli infrastructure and/or our asio-based socketsreally don't like it when a socket is closed inside its own notify readcallback. So instead, let ReportNetError disconnect the auth connectionvia the error callback, which runs asynchronously on the main thread.
Welp, I just hit a deadlock with these changes active: the main thread was sending a transaction that sends an auth message (i. e. plNglTrans So we can't just keep the socket locked during the entire notify callback. I suppose I could revert the locking changes - in my testing,8a73bfc on its own already avoids the crash, but without the extra locking, the crash could still occur with some unlucky timing/scheduling... |
The client reliably crashes after the server sends a
Auth2Cli_KickedOffmessage. I assume this went unnoticed because DIRTSAND never sends that message and even on other servers it doesn't occur in normal gameplay.I spent a while staring at this in the debugger. Apparently the underlying issue is that the auth server socket is closed by its own notify callback (by the
NetCliAuthDisconnectcall inRecvMsg<Auth2Cli_KickedOff>). The async socket code doesn't notice the disconnect in this case, so the disconnect notify callback is never called and the auth connection isn't shut down properly. This causes the client to hang on exit, until it tries to send something to the auth server (usually a ping), which then crashes from trying to write to a closed socket.This PR works around the issue by closing the auth connection asynchronously outside the notify callback, and tightening the locking in the async socket code so nothing else can manipulate a socket while its callbacks are running. Thisseems to fix the hang/crash, but I'm not confident that this is a complete or good solution, so I've made this a draft for now.
I feel like the right solution would be to fire the close notify callback some other way, without relying on the read operation to detect that the socket was closed, but I have no idea how...