Forward subscription cancellation from client to server #47555
Conversation
|
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
alerosmile
changed the title
alerosmile
force-pushed
the
mutiny-cancel-grpc
branch
from
c3db242 to
5afa7d3
Compare
This comment has been minimized.
This comment has been minimized.
|
Plenty of CI failures …
…On Mon, 28 Apr 2025 at 10:31, Georgios Andrianakis ***@***.***> wrote:
@geoand <https://github.com/geoand> requested your review on: #47555
<#47555> Forward subscription
cancellation from client to server.
—
Reply to this email directly, view it on GitHub
<#47555 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACRA6F2GWJYPJQKP3UXJTD23XRP7AVCNFSM6AAAAAB34J24R6VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJXGQZDSOBVG43DKMY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Sorry for this, I was learning about the backpressure interface in gRPC.
|
@alesj Sorry, forgot to remove some test code. I tried to find a solution for the back pressure handling. |
This comment has been minimized.
This comment has been minimized.
|
I've set this to draft because I'll provide some integration tests for this. |
|
@cescoffier please see #41776 (comment). What are the classes |
| // If this is set, the Multi was terminated by completion or failure, not by cancellation. | ||
| private final AtomicBoolean terminated = new AtomicBoolean(); | ||
|
|
||
| private AtomicReference<ClientCallStreamObserver<I>> requestStreamObserver = new AtomicReference<>(); |
There was a problem hiding this comment.
You might be able to condense the 2 atomic variables into one (requestStreamObserver, when null it means terminated)
There was a problem hiding this comment.
Is it guaranteed that beforeStart is called in every case? If not, the emitter will never be terminated on error (after the change, emitter.fail() is only called if requestStreamObserver is not null).
|
@cescoffier I've got an issue with this. If Vert.x is used on the server side ( See io.vertx.core.http.impl.Http2ServerRequest.handleReset(long): If this cannot be changed, this PR makes no sense because it doesn't work if Vert.x is used on the server side. |
O2N and Vertx tests are disable because they fail (see quarkusio#47555 (comment) for details)
|
Added some integration test. The tests in which Vert.x is used on the server side are disabled because they fail (see #47555 (comment)). |
|
Is this ready to be reviewed? |
|
@geoand no, unfortunately not yet. I haven't found the time to comment on eclipse-vertx/vert.x#5562. This has to be solved otherwise this PR doesn't make any sense (see my previous comments here). Is anybody from the Quarkus core team here to help on the Vert.x issue? |
This PR forwards the cancellation events from response
UniandMultito the server by cancelling the gRPC stream.