search

PretendoNetwork / nex-go

Barebones PRUDP/NEX server library written in Go
GNU Affero General Public License v3.0
74 stars 16 forks source link

[Bug]: Gracefully handle relogs and timeouts (possible mishandling of sessions/connections?) #52

Open jonbarrow opened 6 months ago

jonbarrow commented 6 months ago

Checked Existing

What happened?

Currently when a user logs in and out there is a chance for the server to treat the client as still connected. This can be seen in Imora's network dumps, where the console relogs quickly and packets from the server are still being sent.

What did you expect to happen?

The server should probably have a better timeout mechanism for this? I'm unsure how to acomplish this though. We did not see these kinds of issues in Imora's dumps when he used Nintendo Network, so clearly they're doing something better.

https://github.com/PretendoNetwork/nex-go/pull/48 may also help here? Relogs won't matter if we just reset the client state whenever a new SYN is sent.

However we may also need to change how we handle sessions? Looking back over the code, it seems like this is half-baked. PRUDPConnection represents a single PRUDP connection, and has a reference to SocketConnection. SocketConnection represents a physical socket connection (UDP or WebSocket), and has a reference to all the PRUDP connections attached to it. However I can't seem to find a place where we actually set the PRUDPConnection on the SocketConnection?

SocketConnection.Connections comment says:

Open PRUDP connections separated by rdv::Stream ID, also called "port number"

However when this is used in PRUDPConnection.cleanup the connections session ID is used?

pc.Socket.Connections.Delete(pc.SessionID)

This seems very wrong, and is at least partially to blame for some issues. We seem to be mishandling sessions/connections here.

Steps to reproduce? (OPTIONAL)

No response

Any other details to share? (OPTIONAL)

No response

DaniElectra commented 6 months ago

We have previously removed the timeout DISCONNECT packets, but I think these could help when debugging this type of issues, so that we can catch relogs on network dumps easily and prevent confusion with other network issues from the user

The SocketConnection being unused also seems interesting

jonbarrow commented 6 months ago

We have previously removed the timeout DISCONNECT packets

I agree with reimplementing these. But I don't think they will fix all the issues. Pablo has mentioned that Mario Kart 7 does not handle these kinds of packets from the server the same way other games do, so it may be somewhat unreliable to lean on

Also it seems like timeouts just aren't happening correctly in general? Sending the DISCONNECT packet works in cases where

  1. The client is connected
  2. The server knows to time the client out

But according to dumps from Imora, the server seems to still be trying to send the client DATA packets long after the server should have timed out the connection? Which makes me think it wouldn't be sending the DISCONNECT packet at all in those cases