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

Peer Communication Mixes Packets #86

Closed
robert-cronin opened this issue Aug 19, 2020 · 4 comments · Fixed by #84
Closed

Peer Communication Mixes Packets #86

robert-cronin opened this issue Aug 19, 2020 · 4 comments · Fixed by #84
Assignees
Labels
bug Something isn't working development Standard development

Comments

@robert-cronin
Copy link
Contributor

Sometimes you get the wrong message type being received from a request to peer when sending multiple requests at once. This should not happen and is because we're not handling the sockets properly. Right now, we are reusing the sockets with nodejs setKeepAlive, but I suspect we are reusing sockets for concurrent requests and not discriminating between the end of the request/response cycle. This could be done by either attaching a random string to the request packet so it can look for it in the response, or only reassigning a socket once the request/response cycle is finished.

That being said, I suspect this won't be an issue until we get a large number of concurrent requests to a single peer.

@robert-cronin robert-cronin added bug Something isn't working development Standard development labels Aug 19, 2020
@robert-cronin robert-cronin self-assigned this Aug 19, 2020
@robert-cronin
Copy link
Contributor Author

robert-cronin commented Aug 19, 2020

This bug turned up on direct p2p communication and via an intermediary TURN Peer when trying to clone multiple vaults concurrently

@CMCDragonkai
Copy link
Member

Can you explain what is your protocol being used when sending messages? I thought it would be GRPC and we shouldn't have to deal with multiplexing problems. You're not using raw TCP frames right?

@robert-cronin
Copy link
Contributor Author

robert-cronin commented Aug 19, 2020

originally, all peer sharing was done via gRPC, but I changed it halfway through implementing the TURN server as gRPC is not byo socket and we need that to control the sockets.

Now that I have a working implementation of packet relay, I've realised that this is not really the case at all as we don't need to access the sockets of the GitServer/GitClient, we just need to relay the packets.

I will be changing the git transport back to using gRPC shortly.

@robert-cronin
Copy link
Contributor Author

background on gRPC load balancing, might be relevant here: https://grpc.io/blog/grpc-load-balancing/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working development Standard development
Development

Successfully merging a pull request may close this issue.

2 participants