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
fix: create new readers and writer for every async request #10026
Conversation
20b88d4
to
cade08f
Compare
cade08f
to
7feedba
Compare
super( | ||
() -> requestReader, | ||
() -> responseWriter, | ||
new Supplier<>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Looks weird but I found no other way to define a supplier that always returns the same instance of errorResponseWriter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. Didn't see this comment before my review.
I think it is ok if it creates a new instance everytime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
new Supplier<>() { | ||
private final ErrorResponseWriter errorResponseWriter = new ErrorResponseWriter(); | ||
|
||
@Override | ||
public ErrorResponseWriter get() { | ||
return errorResponseWriter; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new Supplier<>() { | |
private final ErrorResponseWriter errorResponseWriter = new ErrorResponseWriter(); | |
@Override | |
public ErrorResponseWriter get() { | |
return errorResponseWriter; | |
} | |
() -> new ErrorResponseWriter() |
final var responseWriter = responseWriterSupplier.get(); | ||
final var errorResponseWriter = errorResponseWriterSupplier.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 I still think you don't have to create these instances here. Instead it can be created in the implementation of handleAsync
. But this is ok for now. We can discuss it when you refactor this class.
I'll merge this directly once jenkins runs through. |
10037: fix: create new readers and writer for every sync request r=oleschoenburg a=oleschoenburg ## Description Applies the fix in #10026 for sync api handlers too. Due to our actor scheduler, even sync handlers can't reuse readers andwriters because `onComplete` on the already completed future doesn't run immediately, causing a data race on the readers and writers when handling concurrent requests. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10014 Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
10037: fix: create new readers and writer for every sync request r=oleschoenburg a=oleschoenburg ## Description Applies the fix in #10026 for sync api handlers too. Due to our actor scheduler, even sync handlers can't reuse readers andwriters because `onComplete` on the already completed future doesn't run immediately, causing a data race on the readers and writers when handling concurrent requests. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10014 Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
10037: fix: create new readers and writer for every sync request r=oleschoenburg a=oleschoenburg ## Description Applies the fix in #10026 for sync api handlers too. Due to our actor scheduler, even sync handlers can't reuse readers andwriters because `onComplete` on the already completed future doesn't run immediately, causing a data race on the readers and writers when handling concurrent requests. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #10014 Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
Description
This allows async api handlers to get new instances of request readers and writers on every request, while synchronous handlers can re-use their writers and readers.
Related issues
closes #10014
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Please refer to our review guidelines.