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

Prevent concurrent execution of the same mutable request object #3197

Merged
merged 6 commits into from
Feb 28, 2025

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

Our HttpRequestMetaData object is mutable, and we expect users to create a new request every time they need to make a new call. Sequential retries are acceptable, but concurrent execution can corrupt internal state. While these expectations are more clear for HTTP users, with gRPC it gets less obvious that they can not subscribe to the same returned Single<Message> concurrently.

Modifications:

  • Enhance FilterableClientToClient to protect users from concurrent execution of the same request, while still allowing sequential retries.
  • Verify concurrent execution is not allowed for HTTP and gRPC.

Result:

Users get RejectedSubscribeException if they subscribe to the same Single that shares underlying meta-data object concurrently. This is the best effort to let users know they misused the client.

Risk:

The change may unexpectedly break existing use-cases. To give users some time to adjust their code, we temporarily introduce a system property to opt-out from this new behavior: -Dio.servicetalk.http.netty.skipConcurrentRequestCheck=true.

Motivation:

Our `HttpRequestMetaData` object is mutable, and we expect users to
create a new request every time they need to make a new call. Sequential
retries are acceptable, but concurrent execution can corrupt internal
state. While these expectations are more clear for HTTP users, with gRPC
it gets less obvious that they can not subscribe to the same returned
`Single<Message>` concurrently.

Modifications:
- Enhance `FilterableClientToClient` to protect users from concurrent
execution of the same request, while still allowing sequential retries.
- Verify concurrent execution is not allowed for HTTP and gRPC.

Result:

Users get `RejectedSubscribeException` if they subscribe to the same
Single that shares underlying meta-data object concurrently.
@idelpivnitskiy idelpivnitskiy self-assigned this Feb 21, 2025
Copy link
Contributor

@bryce-anderson bryce-anderson left a comment

Choose a reason for hiding this comment

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

I don't really like the synchronization so if someone has a better idea I'm interested to hear it. However, I can't think of a better solution that doesn't break behavior so I'm fine with the compromises this makes since it's so easy to back out of the change (both in terms of flag and in terms of we can easily revert and go back to the drawing board if this isn't effective enough etc).

@mgodave
Copy link
Contributor

mgodave commented Feb 25, 2025

I don't really like the synchronization so if someone has a better idea I'm interested to hear it. However, I can't think of a better solution that doesn't break behavior so I'm fine with the compromises this makes since it's so easy to back out of the change (both in terms of flag and in terms of we can easily revert and go back to the drawing board if this isn't effective enough etc).

My initial reaction was that I wasn't thrilled about the mutability and I didn't really have anywhere to go from there having not directly thought about this problem. I think this is probably about as good as we can achieve otherwise.

@idelpivnitskiy idelpivnitskiy merged commit 7a7341a into apple:main Feb 28, 2025
11 checks passed
@idelpivnitskiy idelpivnitskiy deleted the request-lock branch February 28, 2025 19:31
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.

4 participants