Skip to content

UnorderedThreadPoolEventExecutor consumes 100% CPU when idle#6518

Closed
normanmaurer wants to merge 1 commit into
4.1from
unordered
Closed

UnorderedThreadPoolEventExecutor consumes 100% CPU when idle#6518
normanmaurer wants to merge 1 commit into
4.1from
unordered

Conversation

@normanmaurer
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer commented Mar 8, 2017

Motivation:

When UnorderedThreadPoolEventExecutor.execute / submit etc is called it will consume up to 100 % CPU even after the task was executed.

Modifications:

Add a special wrapper which we will be used in execute(...) to wrap the submitted Runnable. This is needed as ScheduledThreadPoolExecutor.execute(...) will delegate to submit(...) which will then use decorateTask(...). The problem with this is that decorateTask(...) needs to ensure we only do our own decoration if we not call from execute(...) as otherwise we may end up creating an endless loop because DefaultPromise will call EventExecutor.execute(...) when notify the listeners of the promise.

Result:

Fixes [#6507].

@normanmaurer
Copy link
Copy Markdown
Member Author

@Scottmitch PTAL... unfortunally there is really no way for a unit test :(

@Scottmitch
Copy link
Copy Markdown
Member

otherwise we may end up to great -> can you clarify what you mean by this?

@normanmaurer
Copy link
Copy Markdown
Member Author

@Scottmitch actually I have an idea for a unit test.... stay tuned

Motivation:

When UnorderedThreadPoolEventExecutor.execute / submit etc is called it will consume up to 100 % CPU even after the task was executed.

Modifications:

Add a special wrapper which we will be used in execute(...) to wrap the submitted Runnable. This is needed as  ScheduledThreadPoolExecutor.execute(...) will delegate to submit(...) which will then use decorateTask(...). The problem with this is that decorateTask(...) needs to ensure we only do our own decoration if we not call from execute(...) as otherwise we may end up creating an endless loop because DefaultPromise will call  EventExecutor.execute(...) when notify the listeners of the promise.

Result:

Fixes [#6507].
Copy link
Copy Markdown
Member

@Scottmitch Scottmitch left a comment

Choose a reason for hiding this comment

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

lgtm ... be nice to avoid the for loop in the test but it should be "good enough"

@normanmaurer
Copy link
Copy Markdown
Member Author

@Scottmitch yeah... could not fine a better solution for the test as without the loop it sometimes passed.

@normanmaurer
Copy link
Copy Markdown
Member Author

Cherry-picked into 4.1 (c6a3cae) and 4.0 (b32f377)

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.

2 participants