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

feat: socket/channel closed detection #337

Merged
merged 14 commits into from
Jan 6, 2024
Merged

feat: socket/channel closed detection #337

merged 14 commits into from
Jan 6, 2024

Conversation

simbleau
Copy link
Collaborator

@simbleau simbleau commented Sep 23, 2023

The logic to close and check sockets/channels if they are closed.

One thing is noticeably absent: How do we disconnect a specific peer?
I have a client/server topology, and I need a server socket to kick specific clients under certain conditions.

Closes #286

@simbleau
Copy link
Collaborator Author

On further investigation, I think I know a potential way to sever connections.

We would need a new signal type: Disconnect, as such:
image

And in the signaling server topologies, the peer disconnection could be handled:
image

This would require we maintain ownership of the WebSocket object, so we can do ws.close().

That's not currently possible because we split the websocket into a Sink and a Stream, and I'm not well versed enough to know what the correct way to proceed. Do we "reunite them"? Do we only need to drop the Sink? I have no idea. Probably something @johanhelsing or @garryod could help or answer.

image

@simbleau
Copy link
Collaborator Author

simbleau commented Oct 5, 2023

@johanhelsing @garryod thoughts?

garryod
garryod previously requested changes Oct 6, 2023
Copy link
Collaborator

@garryod garryod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on board with the direction, would be good to see a bit more granularity for the socket methods

matchbox_socket/src/webrtc_socket/socket.rs Outdated Show resolved Hide resolved
matchbox_socket/src/webrtc_socket/socket.rs Outdated Show resolved Hide resolved
@johanhelsing
Copy link
Owner

Why does signaling need to be involved in this? couldn't the server socket just close the connection?

@simbleau
Copy link
Collaborator Author

simbleau commented Oct 8, 2023

Why does signaling need to be involved in this? couldn't the server socket just close the connection?

That would be good, but I can't find out how to accomplish closing the socket for a specific peer.

Any ideas @johanhelsing ?

@johanhelsing
Copy link
Owner

Why does signaling need to be involved in this? couldn't the server socket just close the connection?

That would be good, but I can't find out how to accomplish closing the socket for a specific peer.

Any ideas @johanhelsing ?

What's the problem?

Wouldn't socket.disconnect(peer) work?

@simbleau
Copy link
Collaborator Author

simbleau commented Oct 31, 2023

Why does signaling need to be involved in this? couldn't the server socket just close the connection?

That would be good, but I can't find out how to accomplish closing the socket for a specific peer.
Any ideas @johanhelsing ?

What's the problem?

Wouldn't socket.disconnect(peer) work?

So, for WebRtcSocket you're suggesting a .disconnect(peer), which I like for an API.

I'm just unsure how it should be implemented. The problems I'm facing are:

  • WebRtcSocket saves no information relating PeerId to WebRtcChannel. Currently, here is how it is saved: channels: Vec<Option<WebRtcChannel>>. Perhaps we could use a HashMap here instead. Then it would need to be atomically synchronized with peers: HashMap<PeerId, PeerState>,.
  • Once a peer gets disconnected, they would still have a connection with the signaling server, right? Why don't we have an API to disconnect that, too, in cases where Server peer is dually operating as the Signaler? The main concern I have is that a connection to the Signaler will never get cleaned up, and it will pile up until there's a stack overflow and which could be abused to crash the server.

@simbleau simbleau changed the title feat: channel/socket closing feat: socket/channel closed detection Nov 24, 2023
@simbleau
Copy link
Collaborator Author

@johanhelsing Updated to detect is channels and sockets are closed, but removed the socket.disconnect(PeerId) API as a todo since it's hard.

Copy link
Owner

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start for proper disconnection handling :)

Just update the doc comment examples so they use the API they're documenting, and this is good to merge.

Btw, I think socket.any_closed() is probably good enough, change it if you like.

matchbox_socket/src/webrtc_socket/socket.rs Outdated Show resolved Hide resolved
matchbox_socket/src/webrtc_socket/socket.rs Outdated Show resolved Hide resolved
matchbox_socket/src/webrtc_socket/socket.rs Show resolved Hide resolved
matchbox_socket/src/webrtc_socket/socket.rs Show resolved Hide resolved
@simbleau simbleau merged commit 3e36ac5 into johanhelsing:main Jan 6, 2024
12 checks passed
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.

Severing a connection
3 participants