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

[#2426] Not cause busy loop when interrupt Thread of AbstractNioSelector #2868

Closed
wants to merge 1 commit into from

Conversation

normanmaurer
Copy link
Member

Motivation:
Because Thread.currentThread().interrupt() will unblock Selector.select() we need to take special care when check if we need to rebuild the Selector. If the unblock was caused by the interrupt() we will clear it and move on as this is most likely a bug in a custom ChannelHandler or a library the user makes use of.

Modification:
Clear the interrupt state of the Thread if the Selector was unblock because of an interrupt and the returned keys was 0.

Result:
No more busy loop caused by Thread.currentThread().interrupt()

@normanmaurer
Copy link
Member Author

@trustin please review... I would love to have this in all 3.6+ branches and 3.2 and cut a 3.6 and 3.2 release with it for Red Hat to consume .

@ghost
Copy link

ghost commented Sep 5, 2014

Build result for #2868 at 908d815: Success

@ghost
Copy link

ghost commented Sep 9, 2014

Build result for #2868 at 452e5ed: Success

@kylinsoong
Copy link

Thanks norman for your help, this saved us time.

// See https://github.com/netty/netty/issues/2426
if (logger.isDebugEnabled()) {
logger.debug("Selector.select() returned prematurely because " +
"Thread.currentThread().interrupt() was called. Use " +
Copy link
Member

Choose a reason for hiding this comment

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

' .. because the I/O thread has been interrupted. Use shutdown() to shut the NioSelector down.'

@trustin
Copy link
Member

trustin commented Sep 12, 2014

I proposed to rephrase the messages and comments. Besides that, it looks fine to me.

@trustin
Copy link
Member

trustin commented Sep 12, 2014

Once done, please feel free to merge.

@trustin trustin self-assigned this Sep 12, 2014
@trustin trustin added this to the 3.9.5.Final milestone Sep 12, 2014
@trustin trustin added defect and removed defect labels Sep 12, 2014
@normanmaurer
Copy link
Member Author

@trustin I will need to backport this to 3.6 and 3.2 also and cut a release. What else branches I should backport ?

Motivation:
Because Thread.currentThread().interrupt() will unblock Selector.select() we need to take special care when check if we need to rebuild the Selector. If the unblock was caused by the interrupt() we will clear it and move on as this is most likely a bug in a custom ChannelHandler or a library the user makes use of.

Modification:
Clear the interrupt state of the Thread if the Selector was unblock because of an interrupt and the returned keys was 0.

Result:
No more busy loop caused by Thread.currentThread().interrupt()
@ghost
Copy link

ghost commented Sep 12, 2014

Build result for #2868 at 0a374cd: Success

@trustin
Copy link
Member

trustin commented Sep 12, 2014

3.9? :-)

@normanmaurer
Copy link
Member Author

@trustin ok 3.9 , 3.6 and .3.2 (sigh). Will cut a release for 3.6 and 3.2 ... Should I also do for 3.9 ?

@normanmaurer
Copy link
Member Author

@trustin was wrong... only need 3.6 (not 3.2 :))

@trustin
Copy link
Member

trustin commented Sep 12, 2014

No need to release. I just meant this commit should go to the latest stable branches including 3.9. and what about 4.x and master? Are we affected there, either?

@normanmaurer
Copy link
Member Author

No it is fixed already in master and 4.0

Am 12.09.2014 um 10:48 schrieb Trustin Lee notifications@github.com:

No need to release. I just meant this commit should go to the latest stable branches including 3.9. and what about 4.x and master? Are we affected there, either?


Reply to this email directly or view it on GitHub.

@normanmaurer
Copy link
Member Author

Was merged into 3.6 and 3.9 branches.

@kylinsoong
Copy link

Today I debug teiid code(it use netty 3.6.2) find a easy way to reproduce high cpu in 3.6.2, if we run the following code:

        ExecutorService nettyPool = Executors.newCachedThreadPool();
        ChannelFactory factory = new NioServerSocketChannelFactory(nettyPool, nettyPool, 1);
        ServerBootstrap bootstrap = new ServerBootstrap(factory);
        bootstrap.setPipelineFactory(new ChannelPipelineFactory() {

            public ChannelPipeline getPipeline() throws Exception {
                return Channels.pipeline(new SimpleChannelHandler());
            }});
        bootstrap.setOption("keepAlive", Boolean.TRUE);
        Channel serverChanel = bootstrap.bind(new InetSocketAddress(0));

        serverChanel.close();
        nettyPool.shutdownNow();

        Thread.currentThread().sleep(Long.MAX_VALUE);

These code cause 2 threads(netty boss threads and netty worker threads) high cpu(100% cpu retained).

I also verified fixed version 3.6.10.Final(https://github.com/netty/netty/releases/tag/netty-3.6.10.Final), it fixed the issue.

I hope my founding can help others, http://ksoong.org/netty-highcpu/ have more details.

@normanmaurer normanmaurer deleted the fix_busy_loop branch November 3, 2014 06:54
baldersheim added a commit to vespa-engine/vespa that referenced this pull request Apr 21, 2017
bratseth added a commit to vespa-engine/vespa that referenced this pull request Apr 21, 2017
hebo1982 added a commit to hebo1982/motan that referenced this pull request Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants