Skip to content

Schedule commands in current thread context #54187

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

Merged
merged 2 commits into from
Mar 26, 2020

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Mar 25, 2020

Changes ThreadPool's schedule method to run the schedule task in the context of the thread that scheduled the task.

This is the more sensible default for this method, and eliminates a range of bugs where the current thread context is mistakenly dropped.

Closes #17143

@ywelsch ywelsch added >enhancement :Core/Infra/Core Core issues without another label labels Mar 25, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

@ywelsch ywelsch marked this pull request as ready for review March 25, 2020 15:19
@ywelsch ywelsch requested review from jimczi and nik9000 March 25, 2020 15:19
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

+1

It has been a while since I looked at this! The reason I never made this change when I worked on this before was that I thought that some calls to schedule intentionally didn't keep the thread context. I wonder if it'd be best to explicitly throw away the context in the places where we don't want to preserve it. It might be as simple as adding a boolean to schedule or something like that. I hate to change the signature but that'd force you to think about whether or not you want to preserve the context every time you call schedule.

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM, it's hard to say if there are usages today that rely on the current default. Although scratching the context can make more harm than preserving it so I think we're on the right side with this change.

@ywelsch
Copy link
Contributor Author

ywelsch commented Mar 25, 2020

It might be as simple as adding a boolean to schedule or something like that. I hate to change the signature but that'd force you to think about whether or not you want to preserve the context every time you call schedule

I thought about this initially, and looked at a lot of callers, coming to the conclusion that I couldn't find any place where not preserving the calling context would make sense. Adding a boolean would i think add more confusion, and can still be introduced if we have an actual case where we need it.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM then!

@ywelsch ywelsch merged commit 0a34b71 into elastic:master Mar 26, 2020
ywelsch added a commit that referenced this pull request Mar 26, 2020
Changes ThreadPool's schedule method to run the schedule task in the context of the thread
that scheduled the task.

This is the more sensible default for this method, and eliminates a range of bugs where the
current thread context is mistakenly dropped.

Closes #17143
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.

Schedule commands in the current thread context
5 participants