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

Provide Thread creation hook support #86609

Open
qstanczyk mannequin opened this issue Nov 23, 2020 · 5 comments
Open

Provide Thread creation hook support #86609

qstanczyk mannequin opened this issue Nov 23, 2020 · 5 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@qstanczyk
Copy link
Mannequin

qstanczyk mannequin commented Nov 23, 2020

BPO 42443
Nosy @Yhg1s, @gpshead, @tiran, @qstanczyk
PRs
  • bpo-42443 Add Thread Creation Hook to Threading.py #23477
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/gpshead'
    closed_at = None
    created_at = <Date 2020-11-23.15:21:52.572>
    labels = ['type-feature', 'library', '3.10', '3.11']
    title = 'Provide Thread creation hook support'
    updated_at = <Date 2021-05-20.00:38:55.044>
    user = 'https://github.com/qstanczyk'

    bugs.python.org fields:

    activity = <Date 2021-05-20.00:38:55.044>
    actor = 'gregory.p.smith'
    assignee = 'gregory.p.smith'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2020-11-23.15:21:52.572>
    creator = 'stanczyk'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42443
    keywords = ['patch']
    message_count = 5.0
    messages = ['381672', '381712', '381755', '383124', '393993']
    nosy_count = 5.0
    nosy_names = ['twouters', 'gregory.p.smith', 'christian.heimes', 'python-dev', 'stanczyk']
    pr_nums = ['23477']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue42443'
    versions = ['Python 3.10', 'Python 3.11']

    @qstanczyk
    Copy link
    Mannequin Author

    qstanczyk mannequin commented Nov 23, 2020

    For monitoring purposes we would like to have a way of registering newly created Python threads with a runtime. For that reason we ask to add a thread creation hook inside threading.py. There are 2 similar hooks already (_profile_hook and _trace_hook).

    @qstanczyk qstanczyk mannequin added 3.10 only security fixes type-feature A feature request or enhancement labels Nov 23, 2020
    @gpshead gpshead added stdlib Python modules in the Lib dir labels Nov 23, 2020
    @tiran
    Copy link
    Member

    tiran commented Nov 24, 2020

    Besindes tests, PR also needs documentation and a better definition how and when the hook is called.

    • IMO it should be called after profiling and tracing hook, so non-trivial hooks can be profiled and traced.
    • It's important to define and document, which thread runs the hook (calling thread or new thread).
    • Since the hook is designed to monitor thread creation, would it make sense to pass the thread object to the hook?
    • How does the hook behave when the callback raises an exception?
    • Is a single hook good enough or should the API behave more like atexit, which supports an abitrary amount of hooks?
    • Instead of just a creation hook, how about lifetime hooks are also called when a thread ends?

    @qstanczyk
    Copy link
    Mannequin Author

    qstanczyk mannequin commented Nov 24, 2020

    Thanks Christian for looking into this, please find my responses inlined:

    • IMO it should be called after profiling and tracing hook, so non-trivial hooks can be profiled and traced.
      Makes sense, Done.
    • It's important to define and document, which thread runs the hook (calling thread or new thread).
      Will update documentation when we agree upon appropriate API.
    • Since the hook is designed to monitor thread creation, would it make sense to pass the thread object to the hook?
      As it's called within the context of the created thread I guess this is not needed (but as you prefer).
    • How does the hook behave when the callback raises an exception?
      I guess it makes sense to do similar thing as in case of profile/trace hooks (Error in the profile function will cause itself unset). WDYT?
    • Is a single hook good enough or should the API behave more like atexit, which supports an arbitrary amount of hooks?
      Feels like it makes sense to keep API similar to what profile/trace provides, otherwise it would be inconsistent.
    • Instead of just a creation hook, how about lifetime hooks are also called when a thread ends?
      Sounds fine, do you mean two independent hooks (_thread_creation_hook, _thread_termination_hook) or some other form?

    @qstanczyk
    Copy link
    Mannequin Author

    qstanczyk mannequin commented Dec 16, 2020

    Friendly ping - does the proposal sound fine?

    @gpshead
    Copy link
    Member

    gpshead commented May 20, 2021

    > * IMO it should be called after profiling and tracing hook, so non-trivial hooks can be profiled and traced.
    Makes sense, Done.

    I updated the PR do this. (if your "Done" indicated you had done this somewhere, it appears you never pushed your changes to the PR branch on github :)

    > * It's important to define and document, which thread runs the hook
    (calling thread or new thread).
    Will update documentation when we agree upon appropriate API.

    I updated the docstring in the PR.

    > * Since the hook is designed to monitor thread creation, would it make sense to pass the thread object to the hook?
    As it's called within the context of the created thread I guess this is not needed (but as you prefer).

    I updated the PR to do this.

    > * How does the hook behave when the callback raises an exception?
    I guess it makes sense to do similar thing as in case of profile/trace hooks (Error in the profile function will cause itself unset). WDYT?

    I like the consistency. I've updated the PR.

    > * Is a single hook good enough or should the API behave more like atexit, which supports an arbitrary amount of hooks?
    Feels like it makes sense to keep API similar to what profile/trace provides, otherwise it would be inconsistent.

    Agreed. when writing the docs we can mention this. Anyone setting these hooks would be wise to check what's already set and decide if they want to set theirs at all or save that and chain call it or not. It is expected to be rare to set this. Our own use case is effectively platform wide for use in most applications.

    > * Instead of just a creation hook, how about lifetime hooks are also called when a thread ends?
    Sounds fine, do you mean two independent hooks (_thread_creation_hook, _thread_termination_hook) or some other form?

    Lets leave this as a separate feature to add if the need arises. I can imagine uses in threading tests, but that doesn't feel like a strong use case.

    @gpshead gpshead added 3.11 only security fixes labels May 20, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @gpshead gpshead removed 3.11 only security fixes 3.10 only security fixes labels Apr 20, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    Status: No status
    Development

    No branches or pull requests

    2 participants