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

WebsocketOutbound sendClose never returns when nested #444

Closed
rstoyanchev opened this issue Oct 2, 2018 · 4 comments
Closed

WebsocketOutbound sendClose never returns when nested #444

rstoyanchev opened this issue Oct 2, 2018 · 4 comments
Labels
type/bug A general bug

Comments

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Oct 2, 2018

Originally reported for WebFlux in SPR-17306 but I was able to confirm with just Reactor Netty that when a call to out.sendClose is nested within the pipeline for the write publisher, then the close does not complete. The following times out:

DisposableServer server = HttpServer.create()
    .tcpConfiguration(s -> s.host("0.0.0.0"))
    .port(0)
    .handle((req, resp) ->
        resp.sendWebsocket(null, (in, out) -> {
          return out.sendObject(Flux.error(new Throwable())
              .onErrorResume(ex -> out.sendClose(1001, "Going Away"))
              .cast(WebSocketFrame.class));

        }))
    .bind()
    .block();


HttpClient.create().websocket()
    .uri("http://0.0.0.0:" + server.address().getPort())
    .handle((in, out) -> in.receiveFrames().then()).next().block(Duration.ofSeconds(5));

However if the write and close are in a sequence, then it works:

DisposableServer server = HttpServer.create()
    .tcpConfiguration(s -> s.host("0.0.0.0"))
    .port(0)
    .handle((req, resp) ->
        resp.sendWebsocket(null, (in, out) -> {

          Flux<WebSocketFrame> writeFlux = Flux.error(new Throwable())
              .onErrorResume(ex -> Flux.empty())
              .cast(WebSocketFrame.class);

          return out.sendObject(writeFlux)
              .then(Mono.defer(() -> out.sendClose(1001, "Going Away")));

        }))
    .bind()
    .block();

I've confirmed the above with Californium only but the issue was reported against Spring Framework 5.0.9 which is based on Bismuth-SR11.

@violetagg violetagg added the status/need-decision This needs team attention and discussion label Oct 7, 2018
@smaldini smaldini removed the status/need-decision This needs team attention and discussion label Oct 8, 2018
@smaldini
Copy link
Contributor

smaldini commented Oct 8, 2018

With the current implementation this is to be expected as we already have one queued sender and we will wait for this sender to terminate (here your sendObject) to run the publisher created by sendClose.
We need to change our current design for publisher handling to :

  • authorize one publisher sender at a time and fast fail any nested call like yours
  • make sendClose fast track to interrupt any current sending instead of using the send publisher path

I'll see with @violetagg can do a quick work around until such refactor happen to make sendClose different, but nested sendXxx will still have the same problem until we fix the overall internal design for sending.

@smaldini smaldini added this to the 0.8.x Backlog milestone Oct 8, 2018
@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Oct 8, 2018

FWIW a nested send is neither expected nor supported in the Spring WebSocket API. It is also not explicitly validated against, but the API provides only one send method with a Publisher.

The one I'm concerned with is sendClose. If it can't be fixed in the near future, we'll need to document it as a limitation since unlike other send methods, this one is actually expected to be used in addition to a send with publisher.

violetagg added a commit that referenced this issue Oct 18, 2018
Prepare to send a close frame on subscribe then close the
underlying channel. Any previous publisher will be canceled.

Clarify that nesting NettyOutbound#send* methods is not supported.

Related to #444
violetagg added a commit that referenced this issue Oct 18, 2018
Prepare to send a close frame on subscribe then close the
underlying channel. Any previous publisher will be canceled.

Clarify that nesting NettyOutbound#send* methods is not supported.

Related to #444
violetagg added a commit that referenced this issue Oct 18, 2018
Prepare to send a close frame on subscribe then close the
underlying channel. Any previous publisher will be canceled.

Clarify that nesting NettyOutbound#send* methods is not supported.

Related to #444
violetagg added a commit that referenced this issue Oct 19, 2018
Prepare to send a close frame on subscribe then close the
underlying channel. Any previous publisher will be canceled.

Clarify that nesting NettyOutbound#send* methods is not supported.

Related to #444
@rstoyanchev
Copy link
Contributor Author

I can confirm the changes after updating to use Bismuth and Californium snapshots, and adding tests.

@violetagg violetagg modified the milestones: 0.8.x Backlog, 0.7.11.RELEASE Oct 25, 2018
@violetagg violetagg added the type/bug A general bug label Oct 25, 2018
@violetagg
Copy link
Member

Handling of nested send operations will be tracked with #478

violetagg added a commit that referenced this issue Jul 2, 2019
Removing reactive bridge was added as a fix for #444.
The removing functionality forces PublisherSender to invoke cancel
and ChannelOperationsHandler to drain the queued messages.
As PublisherSender functionality is not part of ChannelOperationsHandler
anymore there is no need to remove ChannelOperationsHandler from the pipeline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants