Skip to content

PrioritizedExecutorService: Properly wrap on direct calls to "execute".#11956

Merged
gianm merged 2 commits intoapache:masterfrom
gianm:fix-pes-execute
Nov 22, 2021
Merged

PrioritizedExecutorService: Properly wrap on direct calls to "execute".#11956
gianm merged 2 commits intoapache:masterfrom
gianm:fix-pes-execute

Conversation

@gianm
Copy link
Contributor

@gianm gianm commented Nov 19, 2021

Usually, "execute" is called by methods defined in the superclass
AbstractExecutorService, and the passed-in Runnable has been wrapped
by newTaskFor inside a PrioritizedListenableFutureTask. But this method
can also be called directly, and if so, the same wrapping is necessary
for the delegate to get a Runnable that can be entered into a priority
queue with the others.

I don't think this affects production, because I didn't find any production
code that calls this method. I noticed it while working on another branch
doing something that did end up calling this method on a
PrioritizedExecutorService, so I thought it would be nice to split the fix
into its own patch and test.

Usually, "execute" is called by methods defined in the superclass
AbstractExecutorService, and the passed-in Runnable has been wrapped
by newTaskFor inside a PrioritizedListenableFutureTask. But this method
can also be called directly, and if so, the same wrapping is necessary
for the delegate to get a Runnable that can be entered into a priority
queue with the others.
Copy link
Contributor

@capistrant capistrant left a comment

Choose a reason for hiding this comment

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

Looks good to me.. nice find. The CI initially failed for unrelated thing that failed a bunch of recent PRs (hadoop3 deps resolution issue). I have kicked off the rest of CI, +1 pending it passing.

Copy link
Member

@asdf2014 asdf2014 left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@gianm gianm merged commit d6507c9 into apache:master Nov 22, 2021
@gianm gianm deleted the fix-pes-execute branch November 22, 2021 18:30
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants