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

Connections are not disposed when server is closed #495

Closed
mostroverkhov opened this issue Nov 5, 2018 · 7 comments
Closed

Connections are not disposed when server is closed #495

mostroverkhov opened this issue Nov 5, 2018 · 7 comments
Labels
status/duplicate This is a duplicate of another issue type/enhancement A general enhancement
Milestone

Comments

@mostroverkhov
Copy link
Contributor

mostroverkhov commented Nov 5, 2018

Expected behavior

Connections are disposed on DisposableServer.disposeNow(), and Connection.onDispose() Publisher is terminated

Actual behavior

Connection.onDispose() Publisher is not terminated

Steps to reproduce

Code below is timing out

@Test
 public void nettyServerDisposalIssue() {
 MonoProcessor<Void> serverConnDisposed = MonoProcessor.create();

DisposableServer boundServer =
    TcpServer.create()
        .port(8082)
        .doOnConnection(
            serverConnection -> serverConnection.onDispose()
                .subscribe(serverConnDisposed))
        .bind()
        .block();

Connection clientConnection =
    TcpClient.create()
        .addressSupplier(boundServer::address)
        .connect()
        .block();

boundServer.disposeNow();
serverConnDisposed.block(Duration.ofSeconds(5));

Reactor Netty version

0.8.2.RELEASE

JVM version (e.g. java -version)

Java(TM) SE Runtime Environment (build 1.8.0_191-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.191-b12, mixed mode)

OS version (e.g. uname -a)

Linux desktop 4.4.0-138-generic #164~14.04.1-Ubuntu SMP Fri Oct 5 08:56:16 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

@smaldini
Copy link
Contributor

smaldini commented Nov 6, 2018

Interestingly this is a costly thing to do and thats why Netty doesnt do it by default.
We would need to attach a callback for all connections (and remove them when they are disconnected). I'm not sure this should be a default behavior but something like an option could enable unless we find another algorithm. Would be worth asking Netty team too.

@smaldini
Copy link
Contributor

smaldini commented Nov 6, 2018

Looking a bit further, Netty will mark the channels as closing if we shutdown the worker loops from the server (which we don't because they can be shared). I wonder if we should force such shutdown when a server is disposed (and let LoopResources rehydrate new loops for following use).

violetagg added a commit that referenced this issue Nov 14, 2018
…ableServer#dispose

When using the server with the default Tcp/HttpResources, when the server is disposed,
they also will be disposed thus the children channels will be closed also.
@violetagg violetagg modified the milestones: 0.8.3.RELEASE, 0.8.x Backlog, 0.8.4.RELEASE Nov 20, 2018
@dconnelly
Copy link

Does this mean we expect outstanding connections to be abruptly terminated when a server is disposed? If so, should we consider adding support for graceful shutdown with connection draining?

@violetagg
Copy link
Member

@dconnelly We plan to shutdown the EventLoopGroup invoking EventExecutorGroup#shutdownGracefully(). Do you need a specific configuration for the quiet period and the timeout?

@dconnelly
Copy link

@violetagg This is what I was hoping would happen thanks for the confirmation. Default values (2 second quiet period, 15 second timeout) are fine for my use case at least.

@violetagg
Copy link
Member

Approach with channel group will be implemented for TCP/HTTP server (as described here for the client):
#581 (comment)

As a workaround for now - doOnConnection and channelGroup.add(connection.channel()) can be used.

This issue will depend on #586

152476f is a test case using channelGroup approach

@violetagg violetagg added this to the 0.9.6.RELEASE milestone Mar 23, 2020
@violetagg violetagg added status/duplicate This is a duplicate of another issue type/enhancement A general enhancement and removed type/bug A general bug labels Mar 23, 2020
@violetagg
Copy link
Member

Feature is made available as part of #1022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/duplicate This is a duplicate of another issue type/enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants