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 threading.Barrier #53023

Closed
kristjanvalur mannequin opened this issue May 20, 2010 · 15 comments
Closed

Add threading.Barrier #53023

kristjanvalur mannequin opened this issue May 20, 2010 · 15 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@kristjanvalur
Copy link
Mannequin

kristjanvalur mannequin commented May 20, 2010

BPO 8777
Nosy @birkenfeld, @kristjanvalur, @giampaolo
Dependencies
  • bpo-10218: Add a return value to threading.Condition.wait()
  • Files
  • barrier.patch
  • barrier3.patch: Patch for py3k
  • barrier4.patch
  • barrier4.patch
  • 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 = None
    closed_at = <Date 2010-10-28.09:45:05.309>
    created_at = <Date 2010-05-20.17:07:28.652>
    labels = ['type-feature', 'library']
    title = 'Add threading.Barrier'
    updated_at = <Date 2011-05-26.16:46:31.727>
    user = 'https://github.com/kristjanvalur'

    bugs.python.org fields:

    activity = <Date 2011-05-26.16:46:31.727>
    actor = 'stutzbach'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-10-28.09:45:05.309>
    closer = 'kristjan.jonsson'
    components = ['Library (Lib)']
    creation = <Date 2010-05-20.17:07:28.652>
    creator = 'kristjan.jonsson'
    dependencies = ['10218']
    files = ['17417', '19379', '19394', '19395']
    hgrepos = []
    issue_num = 8777
    keywords = ['patch']
    message_count = 15.0
    messages = ['106167', '106375', '106446', '119454', '119500', '119667', '119756', '119759', '119760', '119761', '119765', '119766', '119768', '119769', '119770']
    nosy_count = 4.0
    nosy_names = ['georg.brandl', 'kristjan.jonsson', 'giampaolo.rodola', 'jyasskin']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue8777'
    versions = ['Python 3.2']

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented May 20, 2010

    The "barrier" synchronization primitive is often very useful. It is simpler to use than an Event, for example, when waiting for threads to start up, or to finish.
    The patch contains a simple barrier implementation based on a Condition variable, for your perusal.

    See http://en.wikipedia.org/wiki/Barrier_(computer_science) for info.

    This particular implementation contains an important feature: The ability to adjust the 'count' of the barrier. This is useful in case a thread dies for some reason, to avoid a deadlock for the other threads.

    There is still no documentation, since this is only a proposal, but there is a unittest.

    @kristjanvalur kristjanvalur mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 20, 2010
    @jyasskin
    Copy link
    Mannequin

    jyasskin mannequin commented May 24, 2010

    You should probably mention that pthread_barrier and java.util.concurrent.CyclicBarrier are prior art for this. I'm thinking about them when looking at the API to see whether your differences make sense.

    "enter" seems to be the wrong term for this, since there's no matching "exit" call. "wait" or "block" seem better.

    Both pthread_barrier and CyclicBarrier provide a way to identify a unique thread from each group. pthread_barrier_wait returns true in exactly one thread, while CyclicBarrier runs a callback while all other threads are still paused. I'd be inclined to use CyclicBarrier's interface here, although then you have to define what happens when the action raises an exception.

    _release should notify_all after its loop.

    adjust_count makes me nervous. Is there a context manager interface that would make this cleaner/safer?

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented May 25, 2010

    I'll provide a new version shortly, targeted for the py3k branch.

    @birkenfeld
    Copy link
    Member

    birkenfeld commented Oct 23, 2010

    Ping -- is this something you want in 3.2, Kristjan?

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Oct 24, 2010

    Hi, I had forgotten about this.
    I went back to the drawing board and had almost completed a new version. Looking at the Java barrier shows how one can go overboard with stuff. My though with the barrier is to provide a simple synchronization primitive that works well for example in the unittests, without trying too hard to over design it in terms of failure modes.

    Anyway, I´ll get my act together. Same with RWLock, that is almost ready

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Oct 27, 2010

    Okay, here is a new submission.
    I've redesigned it to be more reminiscent of the Java version, by allowing the barrier to have a "Broken" state and raising a BrokenBarrierError.
    I've also redesigned the mechanism from a simple perpetually increasing index of "entered" and "released" into a proper two-state machine which is either "filling" or "draining".

    There is also a rather comprehensive set of tests.

    What is missing is documentation, somethign I shall add if this gets a positive response.

    Note how, in the tests, I sometimes create a "barrier2" object to facilitate external synchronization. This demonstrates the simplicity of using this primitive.

    Another note: In order to implement "timeout" behaviour, I changed Condition.wait() to return True in case it returns due to a timeout occurring. I folded this into this patch, but if such a change is not accepted, or we want it separately, then I'll have to remove the timeout functionality from the Barrier. I don't want to have complicated logic in there to measure time. Also, I do think that locking primitives that time out should be able to provide an indication to that fact to their callers, so condition.wait() really should do that.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Oct 28, 2010

    ping?

    @birkenfeld
    Copy link
    Member

    birkenfeld commented Oct 28, 2010

    The tests pass for me, and the patch looks good except for a stray change to Condition objects.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Oct 28, 2010

    Right. The condition object change is necessary to have timeout work. I can remove that feature, and slate it for another day. Add a separate patch for a Condition.wait() return value. All of the other apis are able to let the caller know whether a timeout occurred or not, I think Condition.wait() should do the same.

    Actually, I can fudge the timeout with time.clock(), which is good enough.

    I'll write up some docs.

    @birkenfeld
    Copy link
    Member

    birkenfeld commented Oct 28, 2010

    Well, that change would be fine by me, it was just not explained anywhere in the patch. So if it's going to be documented (with versionchanged etc.), just leave it in.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Oct 28, 2010

    Here is an updated patch. It contains documentation.
    ReStructured isn't my Forte, and I don't know how to verify that it is correct, so please review it for me.

    @birkenfeld
    Copy link
    Member

    birkenfeld commented Oct 28, 2010

    Two comments:

    • The return value of wait() isn't documented well. What is the significance of the returned index, i.e. what does distinguish it from a randomly selected one in range(parties)?

    • get_parties() and is_broken() should be properties (waiting, broken), to be consistent with other threading APIs (Thread.name etc). get_waiting() does "real" work (can it block?) and should remain a method.

    Don't worry about markup errors, I review doc changes routinely after they are committed.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Oct 28, 2010

    Right.
    I've provided more text for the return value and provided an example.
    I´ve changed all three to properties, the locking wasn't really required for waiting().
    I added some extra tests for the properties.

    @birkenfeld
    Copy link
    Member

    birkenfeld commented Oct 28, 2010

    Looks good to me now, I think you can commit it.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Oct 28, 2010

    Committed as revision 85878

    @kristjanvalur kristjanvalur mannequin closed this as completed Oct 28, 2010
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 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
    None yet
    Development

    No branches or pull requests

    1 participant