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

Add name to threads created by WorkerPool #7813

Merged

Conversation

Projects
None yet
2 participants
@cattibrie
Copy link
Contributor

commented May 29, 2019

Problem

When debugging pants with an issue related to multithreading there wasn't enough info about threads created by WorkerPool and the workunit that started the WorkerPool.

Solution

When a new thread in the WorkerPool is created the thread receives a name by its parent workunit name.

@cattibrie cattibrie changed the title Add names to threads created by WorkerPool Add name to threads created by WorkerPool May 29, 2019

@blorente blorente requested review from illicitonion, stuhood and blorente and removed request for illicitonion and stuhood May 29, 2019

@cattibrie cattibrie force-pushed the cattibrie:etyurina/fix_multithreading_bug branch from 9c42387 to 0cc5ccb May 29, 2019

@illicitonion
Copy link
Contributor

left a comment

Yay clarity!

@@ -36,12 +36,20 @@ class WorkerPool(object):
may not be effective. Use this class primarily for IO-bound work.
"""

def __init__(self, parent_workunit, run_tracker, num_workers):
def __init__(self, parent_workunit, run_tracker, num_workers, thread_name_prefix):

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 29, 2019

Contributor

Because you always use the workunit name as the prefix, we could probably not add this argument, not change all of the callers, and instead just use parent_workunit.name in this function where you're currently using thread_name_prefix?

This comment has been minimized.

Copy link
@cattibrie

cattibrie May 29, 2019

Author Contributor

For parent_workunit we use workunit.parent and for parent_workunit.name I used workunit.name. A solution could be to pass just the workunit, but background_worker_pool as parent_workunit takes self.get_background_root_workunit() that doesn't have a parent and background_worker_pool threads are not created within workunit context.
So it could be better to stay with the current solution.

@illicitonion illicitonion merged commit f1d5964 into pantsbuild:master May 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.