brainlag / JavaNSQClient

Fast Java client for NSQ
MIT License
90 stars 57 forks source link

Consumer does not close netty connection after invoking shutdown() method #7

Closed rukyzhc closed 8 years ago

rukyzhc commented 8 years ago

I found that after I call the shutdown() method of the NSQConsumer, there is still one thread of the netty nioEventLoopGroup

rukyzhc commented 8 years ago

It seems that you should call connection.close() while try to cleanClose() and then invoke group.shutdownGracefully() in each connection's close() method.

in NSQConsumer.java

    private void cleanClose() {
        final NSQCommand command = NSQCommand.instance("CLS");
        try {
            for (final Connection connection : connections.values()) {
                final NSQFrame frame = connection.commandAndWait(command);
                if (frame instanceof ErrorFrame) {
                    final String err = ((ErrorFrame) frame).getErrorMessage();
                    if (err.startsWith("E_INVALID")) {
                        throw new IllegalStateException(err);
                    }
                }
                connection.close();
            }
        } catch (final TimeoutException e) {
            LogManager.getLogger(this).warn("No clean disconnect", e);
        }
    }

in Connection.java

    public void close() {
        LogManager.getLogger(this).info("Closing  connection: " + this);
        channel.disconnect();
        group.shutdownGracefully();
    }
brainlag commented 8 years ago

I will look into it later today.

brainlag commented 8 years ago

I looked into it and don't think it is a great idea to shut down the EventLoopGroup when a consumer is shutdown, because the group is shared by all consumers. I don't think this is a problem at all since we don't leak EventLoopGroups.

rukyzhc commented 8 years ago

Well, I agree that the EventLoopGroup should not be closed here. But it should still be closed somewhere when I want to stop the program gracefully.

Perhaps I need an additional static method "shutdown" in the Connection.java and call it after cleanClose all connections.

brainlag commented 8 years ago

I think the long term goal is to give the user more control over the the EventLoopGroup.

rukyzhc commented 8 years ago

Agree your suggestion, and I just propose a way that I can close my program :)

brainlag commented 8 years ago

See #10.