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

NetworkManager StopClient Deadlock if Transport.ClientDisconnect doesn't check if already disconnected #2353

Closed
miwarnec opened this issue Oct 21, 2020 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@miwarnec
Copy link
Collaborator

for future reference.
reproduce:

  • kcp2k v1.0
  • build = server only
  • editor (2020.1) = client only
  • editor presses stopclient in networkmanagerhud
  • this happens:
            NetworkManager StopClient 
              KcpTransport.ClientDisconnect()
                KcpConnection.Disconnect
              Kcp OnClientDisconnected
                NetworkManager OnClientDisconnectInternal
                  NetworkManager OnClientDisconnect
                    NetworkManager StopClient

encountered it because kcptransport ClientDisconnect does not check if already disconnected, so it would call it over and over again.

note that kcp clientdisconnect sets clientconnect=null AFTER the call. but the deadlock is IN the call.

in other words we need to avoid the self recursion in networkmanager stopclient.

@miwarnec miwarnec added the bug Something isn't working label Oct 21, 2020
@miwarnec miwarnec self-assigned this Oct 21, 2020
miwarnec added a commit to MirrorNetworking/kcp2k that referenced this issue Oct 21, 2020
miwarnec added a commit that referenced this issue Oct 21, 2020
…ort ClientDisconnect calls OnDisconnected immediately. Related to: #2353
miwarnec added a commit that referenced this issue Oct 21, 2020
…ort ClientDisconnect calls OnDisconnected immediately. Related to: #2353
@miwarnec miwarnec changed the title NetworkManager StopClient Deadlock if Transport.Disconnect doesn't check if already disconnected NetworkManager StopClient Deadlock if Transport.ClientDisconnect doesn't check if already disconnected Oct 22, 2020
@miwarnec
Copy link
Collaborator Author

should be fixed in master.
we added checks to NetworkClient.OnTransportDisconnected so it's only called once

viacheslavpopov pushed a commit to viacheslavpopov/KCP2K that referenced this issue Aug 20, 2021
@miwarnec miwarnec reopened this Dec 23, 2023
@miwarnec
Copy link
Collaborator Author

reopening this because the fix in kcp was wrong.
it caused another kcp issue: #3704
instead we need to fix this in mirror if it still happens

@miwarnec
Copy link
Collaborator Author

miwarnec commented Dec 23, 2023

nvm this doesn't happen anymore.
if it ever occurs again:

  • StopClient should call disconnect
  • OnDisconnected should never stop client

this is similar to very old UNET bugs/behaviour that we fixed the same way:

  • DoSomething does something
  • OnDidSomething never ever should call DoSomething again

miwarnec added a commit that referenced this issue Dec 23, 2023
…eeded anymore since the original Mirror issue is long gone
miwarnec added a commit that referenced this issue Jan 3, 2024
- added [KCP] to all log messages
- fix: #3704 remove old fix for #2353 which caused log spam and isn't needed anymore since the
  original Mirror issue is long gone
- fix: KcpClient.RawSend now returns if socket wasn't created yet
- fix: #3591 KcpPeer.SendDisconnect now rapid
  fires several unreliable messages instead of sending reliable. Fixes disconnect message not
  going through if the connection is closed & removed immediately after.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant