Skip to content

Conversation

@bentsku
Copy link

@bentsku bentsku commented May 30, 2024

We got a report from a customer that running a migration with diesel would hang.

After investigating, it seems running an insert query of approximately 25MiB would just hang.

I've tracked down the reason for it: the flow of our proxy is flawed. Basically, we wait on the "receiving" socket, in this migration case, the client, so that it sends data. We then construct our packet, and once this packet is complete, we intercept it and populate the "outbound" packet to be sent to the server (the packet sent by the client and intercepted).
However, we only call Socket.send once there, and do the right thing by slicing the outbound data with what we really sent. However, to get to send again, we need the client to send more data. But the client is waiting for the server to respond, and the server hasn't received anything (or just a partial send, not a full query).

Until we refactor the proxy deeper, the fix consist of iterating over the outbound data until there's none left. I've tested it locally and it solved the issue.

As an alternative solution, using Socket.sendall could also work, and we should then remove the whole logic around slicing the outbound data with the sent length.

@bentsku bentsku requested a review from whummer May 30, 2024 16:47
@bentsku bentsku self-assigned this May 30, 2024
@bentsku bentsku force-pushed the fix-socket-partial-send branch from deb6175 to 033f27c Compare May 30, 2024 16:47
@bentsku bentsku requested review from dfangl and steffyP May 30, 2024 18:34
Copy link
Member

@steffyP steffyP left a comment

Choose a reason for hiding this comment

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

Thanks so much for investigating, @bentsku! 🚀
That's a great catch!

@bentsku bentsku merged commit 05d08ba into master Jun 3, 2024
@bentsku bentsku deleted the fix-socket-partial-send branch June 3, 2024 15:25
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.

3 participants