Skip to content

NIO read spin event loop spin when half closed#7801

Merged
normanmaurer merged 3 commits into
netty:4.1from
Scottmitch:nio_read_halfclose
Mar 28, 2018
Merged

NIO read spin event loop spin when half closed#7801
normanmaurer merged 3 commits into
netty:4.1from
Scottmitch:nio_read_halfclose

Conversation

@Scottmitch
Copy link
Copy Markdown
Member

@Scottmitch Scottmitch commented Mar 21, 2018

Motivation:
AbstractNioByteChannel will detect that the remote end of the socket has
been closed and propagate a user event through the pipeline. However if
the user has auto read on, or calls read again, we may propagate the
same user events again. If the underlying transport continuously
notifies us that there is read activity this will happen in a spin loop
which consumes unnecessary CPU.

Modifications:

  • AbstractNioByteChannel's unsafe read() should check if the input side
    of the socket has been shutdown before processing the event. This is
    consistent with EPOLL and KQUEUE transports.

Result:
No more read spin loop in NIO when the channel is half closed.

@Scottmitch
NIO read spin event loop spin when half closed
51607ea 
Motivation:
AbstractNioByteChannel will detect that the remote end of the socket has
been closed and propagate a user event through the pipeline. However if
the user has auto read on, or calls read again, we may propagate the
same user events again. If the underlying transport continuously
notifies us that there is read activity this will happen in a spin loop
which consumes unnecessary CPU.

Modifications:
- AbstractNioByteChannel's unsafe read() should check if the input side
of the socket has been shutdown before processing the event. This is
consistent with EPOLL and KQUEUE transports.

Result:
No more read spin loop in NIO when the channel is half closed.
@Scottmitch Scottmitch added this to the 4.1.23.Final milestone Mar 21, 2018
@Scottmitch Scottmitch self-assigned this Mar 21, 2018
@Scottmitch Scottmitch requested a review from normanmaurer March 21, 2018 19:26
@Scottmitch Scottmitch mentioned this pull request Mar 21, 2018
Closed
@Scottmitch
Copy link
Copy Markdown
Member Author

Scottmitch commented Mar 21, 2018

@RoganDawes - can you verify this PR fixes the issue you observed with NIO?

@Scottmitch
Copy link
Copy Markdown
Member Author

Scottmitch commented Mar 21, 2018

unit tests coming soon.

@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Mar 26, 2018

@RoganDawes ping

@Scottmitch
Copy link
Copy Markdown
Member Author

Scottmitch commented Mar 28, 2018

big thanks to @normanmaurer for the unit test ... sorry for the delay!

normanmaurer
normanmaurer reviewed Mar 28, 2018
View reviewed changes
Comment thread transport/src/main/java/io/netty/channel/nio/AbstractNioByteChannel.java Outdated

private static boolean isAllowHalfClosure(ChannelConfig config) {
return config instanceof DefaultSocketChannelConfig &&
((DefaultSocketChannelConfig) config).isAllowHalfClosure();
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer Mar 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace DefaultSocketChannelConfig with SocketChannelConfig. After this its ready to be merged :)

normanmaurer
normanmaurer requested changes Mar 28, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change... after this LGTM

@RoganDawes
Copy link
Copy Markdown
Contributor

RoganDawes commented Mar 28, 2018 •
edited
Loading

Sorry for the delay. I'm actually not quite sure how to test if this PR solves my problem. I pulled the latest changes, but this doesn't seem to be present, not sure how to get this branch to test 😳

FWIW, I added a

    public void userEventTriggered(final ChannelHandlerContext ctx, final Object evt) throws Exception {
        if (evt == ChannelInputShutdownReadComplete.INSTANCE) {
    +                       ctx.channel().config().setAutoRead(false);
           System.out.print(".");
            // ignore
    } else

to one of the handlers in the channel, and the problem seems to have gone away.

@normanmaurer normanmaurer merged commit ed06683 into netty:4.1 Mar 28, 2018
@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Mar 28, 2018

@RoganDawes please just pull the latest 4.1 branch... it was just merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@normanmaurer normanmaurer normanmaurer requested changes

Assignees

@Scottmitch Scottmitch

Labels

defect

Projects

None yet

Milestone

4.1.23.Final

Development

Successfully merging this pull request may close these issues.

3 participants

@Scottmitch @normanmaurer @RoganDawes