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

peer2peer network always receives messages with a timeout #61

Open
heckj opened this issue Apr 27, 2024 · 2 comments
Open

peer2peer network always receives messages with a timeout #61

heckj opened this issue Apr 27, 2024 · 2 comments
Assignees
Labels
bug Something isn't working testing testing of existing code or functionality

Comments

@heckj
Copy link
Collaborator

heckj commented Apr 27, 2024

I'm not 600% this is a fatal flaw, but it's an unintended discrepancy in the network providers.

For the WebSocket, only the "handshake" phase has an explicit timeout, and the ongoing process of receiving messages is not raced against a timeout. The idea being there may be extended periods of time (seconds, minutes, etc) where the connection is established, but nothing needs to be synced on the connection. With a timeout, the "waiting for a next message" on the connection would throw an error, and in the ongoing loop that would terminate the connection - potentially to see it attempt to reconnect if that was configured.

In the Peer2Peer provider, both the peering handshake process (both accepting a new connection and initiating a new connectIon) should use a timeout - we expect a timely handshake to establish the active connection, but there-after the ongoing loop - in either case - should require a timeout.

@heckj heckj added the bug Something isn't working label Apr 27, 2024
@heckj heckj self-assigned this Apr 27, 2024
@alexjg
Copy link

alexjg commented Apr 27, 2024

FWIW the web socket server will send a ping (the web socket ping message that is) every so often (every 5s at the moment I think) and if the client does not respond with a pong within a timeout then the server closes the connection.

@heckj
Copy link
Collaborator Author

heckj commented Apr 27, 2024

Yeah, I was recalling something like that as well. I added a to-do item for myself to create a test to validate everything is working as expected with #62 - I'm not entirely clear on if or how URLSessionWebSocketDataTask does pings, and if that connection is meant to be retained with a heartbeat, if I need to add something explicit to do so.

Then there's a broader question of would it be better to drop the connection while things are idle, and attempt to reconnect when it finds a change and wants to attempt to sync it. Right now that's not an option, but maybe it should be? At least with a configurable setting.

@heckj heckj added the testing testing of existing code or functionality label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing testing of existing code or functionality
Projects
None yet
Development

No branches or pull requests

2 participants