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 _PyOnceFlag_CallOnceTimed #115229

Closed
mpage opened this issue Feb 9, 2024 · 5 comments
Closed

Add _PyOnceFlag_CallOnceTimed #115229

mpage opened this issue Feb 9, 2024 · 5 comments
Labels
type-feature A feature request or enhancement

Comments

@mpage
Copy link
Contributor

mpage commented Feb 9, 2024

Feature or enhancement

Proposal:

We may want to call a function once, but only wait for a given amount of time for the function to be called. For example, to extend the join operation in #115190 with a timeout argument, we need to be able to bound the amount of time we wait for the join operation to be called (in addition to how long the join takes once it's been called).

It seems like the natural place for such logic to live is in _PyOnceFlag, otherwise we would need to duplicate the _PyOnceFlag functionality and extend it to support timeouts. That said, I'm not sure how widely applicable this will be, so it may make sense to just duplicate the logic. Assuming we want to support such a use case, I propose adding _PyOnceFlag_CallOnceTimed with the following semantics:

typedef enum {
     // Successfully called the function
     Py_CALL_ONCE_OK = 0,

     // Called the function, but it failed
     Py_CALL_ONCE_FAILED = -1,

     // Timed out waiting to call the function
     Py_CALL_ONCE_TIMEOUT = -2,
  
     // Interrupted while waiting to call the function
     Py_CALL_INTR = -3,
 } _PyCallOnceTimedResult;

// Calls `fn` once using `flag`. The `arg` is passed to the call to `fn`.
//
// The `timeout_ns` argument specifies the maximum amount of time to wait for `fn` to be 
// called, with -1 indicating an infinite wait.
//
// If `fn` returns 0 (success), then subsequent calls immediately return 0.
// If `fn` returns -1 (failure), then subsequent calls will retry the call.
static inline _PyCallOnceTimedResult
_PyOnceFlag_CallOnceTimed(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg, _PyTime_t timeout_ns);

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

@mpage mpage added the type-feature A feature request or enhancement label Feb 9, 2024
@mpage
Copy link
Contributor Author

mpage commented Feb 9, 2024

@colesbury - I wanted to get your take on this. Happy to implement if you think it's worth having.

@colesbury
Copy link
Contributor

I think we should be able to get away with PyEvent_WaitTimed. It seems like thread joining needs two stages because pthread_join() doesn't take a timeout.

  1. PyEvent_WaitTimed(event, timeout); where the event is set when thread is about to exit.
  2. _PyOnceFlag_CallOnce(...) to subsequently call PyThread_join_thread() (no timeout because we can't pass a timeout to join)

@mpage
Copy link
Contributor Author

mpage commented Feb 10, 2024

I might be misunderstanding, but I think that only works if we allow detach() calls to jump ahead of join() calls that are already in progress. Right now we're using _PyOnceFlag to serialize all operations on a ThreadHandle.

It sounds like there probably isn't enough justification for this, though, so I can just implement support directly in ThreadHandle.

@colesbury
Copy link
Contributor

I think the implementation would be simpler if we don't expose detach(). We only really need to call it from the destructor, and that doesn't require synchronization.

If we continue to expose detach() then I think detach() calls jumping ahead of join() makes sense to me. Otherwise, detach() can block for a long time or even deadlock. But I think this is a all extra complexity without a clear purpose and that we should instead not expose detach() except possibly for testing purposes.

@mpage
Copy link
Contributor Author

mpage commented Feb 10, 2024

But I think this is a all extra complexity without a clear purpose and that we should instead not expose detach() except possibly for testing purposes.

Makes sense to me. I wasn't sure why it was there either. It looks like it's only used in test_thread and that only tests the interaction between join() and detach().

@mpage mpage closed this as completed Feb 10, 2024
@mpage mpage reopened this Feb 10, 2024
@mpage mpage closed this as not planned Won't fix, can't repro, duplicate, stale Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants