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

soroban-rpc: fix datarace in bufferedResponseWriter.WriteOut #961

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

tsachiherman
Copy link
Contributor

@tsachiherman tsachiherman commented Sep 13, 2023

What

Fix data race #959 .

Why

When implementing bufferedResponseWriter.WriteOut, I attempted to implement the writing to the output stream asynchronously, so that in case of context expiration the writing to the channel would be suspended at once.

Unfortuntally, it led to conflict with go http server library : When the bufferedResponseWriter.WriteOut could not complete writing to the stream in time, the method would return. However, the writing operation was still going on it's own pace.
then, the http server was attempting to write to the stream, creating a data race.

The solution is to write to the stream in smaller chunks, and continuously check the ctx.Err in order to examine when it expires.

Known limitations

n/a

@tsachiherman tsachiherman self-assigned this Sep 13, 2023
@tsachiherman tsachiherman added this to the Soroban Testnet Release milestone Sep 13, 2023
@tsachiherman tsachiherman linked an issue Sep 13, 2023 that may be closed by this pull request
@2opremio
Copy link
Contributor

2opremio commented Sep 13, 2023

I would advocate to simply writing all at once. I think responses are small, so I think we can afford to surpass the deadline by the writing time.

@tsachiherman
Copy link
Contributor Author

I would advocate to simply writing all at once. I think responses are small, so I think we can afford to surpass the deadline by the writing time.

I'd like to keep it for protection against malicious client leaving the connection open and draining it at a low rate.

@2opremio
Copy link
Contributor

2opremio commented Sep 13, 2023

Chunks are not going to help you with that either, they can block when reading a single byte

And the MTU doesn't help in all cases, making an assumption on the link layer is overkill at this level of abstraction

@tsachiherman tsachiherman enabled auto-merge (squash) September 13, 2023 18:26
@mollykarcher mollykarcher removed this from the Soroban Testnet Release milestone Sep 13, 2023
@tsachiherman tsachiherman merged commit d3664b5 into main Sep 13, 2023
21 of 22 checks passed
@tsachiherman tsachiherman deleted the tsachi/fixdatarace-WriteOut branch September 13, 2023 20:17
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.

soroban-rpc: data race found by TestCLIContractDeploy
4 participants