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

Close connection on transform error #707

Merged
merged 4 commits into from
Jul 26, 2022

Conversation

rukai
Copy link
Member

@rukai rukai commented Jul 19, 2022

Discovered while investigating #706 but quite likely unrelated.

I had previously expected that when a transform returns a Result::Err the connection would be immediately dropped.
Upon writing an integration test to prove this, I discovered that it is in fact not the case.
Currently a transform returning Result::Err will result in no reply sent to the client and the connection remaining open, very likely resulting in the client waiting indefinitely for a response.

So this PR changes shotover to drop the connection immediately when a transform returns a Result::Err.
If a transform wishes to avoid dropping the connection when encountering an error they should handle the error internally.

There was also some changes needed to ensure the send/receive tasks are terminated when the main task is terminated

flushing

I hit an issue where the final messages sent were being lost.
The fix for that was to flush any pending messages before terminating the send task.

This was very tricky to track down because of the connection closing logic needed by cassandra version handling.
It looked like the problem was caused by a bug in that complicated logic, but it was actually that that logic was often exposing the bug due to its timing of message sending -> connection close.

@rukai rukai changed the title Drop connection on transform error Close connection on transform error Jul 19, 2022
@rukai rukai force-pushed the drop_connection_on_error branch 6 times, most recently from f673ac6 to 50c6ba4 Compare July 20, 2022 06:01
@rukai rukai force-pushed the drop_connection_on_error branch 4 times, most recently from 9b25774 to 33e5b28 Compare July 21, 2022 00:11
@conorbros conorbros merged commit a42da2c into shotover:main Jul 26, 2022
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