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

Pass opaque state to PyContext_AddWatcher callback #127124

Open
rhansen opened this issue Nov 22, 2024 · 3 comments
Open

Pass opaque state to PyContext_AddWatcher callback #127124

rhansen opened this issue Nov 22, 2024 · 3 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@rhansen
Copy link
Contributor

rhansen commented Nov 22, 2024

Feature or enhancement

Proposal:

I would like to change the Context watcher C API (new to v3.14, see #119333) to pass an opaque pointer to the callback. Specifically, change:

/*
* Context object watcher callback function. The object passed to the callback
* is event-specific; see PyContextEvent for details.
*
* if the callback returns with an exception set, it must return -1. Otherwise
* it should return 0
*/
typedef int (*PyContext_WatchCallback)(PyContextEvent, PyObject *);
/*
* Register a per-interpreter callback that will be invoked for context object
* enter/exit events.
*
* Returns a handle that may be passed to PyContext_ClearWatcher on success,
* or -1 and sets and error if no more handles are available.
*/
PyAPI_FUNC(int) PyContext_AddWatcher(PyContext_WatchCallback callback);

to:

/*
 * Context object watcher callback function.  arg is the same pointer passed to
 * PyContext_AddWatcher when the callback was registered.  The object passed to
 * the callback is event-specific; see PyContextEvent for details.
 *
 * if the callback returns with an exception set, it must return -1. Otherwise
 * it should return 0
 */
typedef int PyContext_WatchCallback(
    void *arg, PyContextEvent event, PyObject *obj);

/*
 * Register a per-interpreter callback that will be invoked for context events.
 * arg is an optional opaque pointer that is passed back to the callback; it
 * can be used to manage state if desired.
 *
 * Returns a handle that may be passed to PyContext_ClearWatcher on success,
 * or -1 and sets and error if no more handles are available.
 */
PyAPI_FUNC(int) PyContext_AddWatcher(
    PyContext_WatchCallback *callback, void *arg);

The original idea was for the callback to get any required state from the current context, but:

  • The config/state is not guaranteed to be available in the context. For example, the Python code can do ctx = contextvars.Context() instead of ctx = contextvars.copy_context(). Or it can do copy_context(), but early during initialization and squirrel the context away for later use. The latter seems plausible for 3rd party libraries (where the user might not have much control).
  • The same watcher callback cannot be registered multiple times simultaneously, each with its own config/state. This came up while I was refactoring TestContextObjectWatchers to add some more tests for a change I’m working on.

I’m not sure how likely either is to come up in normal use, but I think it is worth changing the design before the 3.14 release cements it.

Has this already been discussed elsewhere?

I have already discussed this feature proposal on Discourse

Links to previous discussion of this feature:

https://discuss.python.org/t/v3-14a1-design-limitations-of-pycontext-addwatcher/68177/4

cc @fried

Linked PRs

@rhansen rhansen added the type-feature A feature request or enhancement label Nov 22, 2024
rhansen added a commit to rhansen/cpython that referenced this issue Nov 22, 2024
This enables users to associate state with the callback without
relying on globals.

Also:
  * Refactor the tests for improved readability and extensibility, and
    to cover the new opaque pointer.
  * Drop the pointer from the `PyContext_WatchCallback` typedef.  This
    de-obfuscates the fact that pointers are involved, and makes it
    possible to forward-declare functions to improve readability:

        static PyContext_WatchCallback my_callback;

        int
        my_callback(void *arg, PyContextEvent event, PyObject *obj)
        {
            ...
        }
@ZeroIntensity
Copy link
Member

void *arg is a bit of a headache, and I'm not sure we have any precedent for it in the C API. There's a few things we need to worry about:

  • How do we deal with thread safety on the state?
  • Who will manage the lifetime of it?
  • What rules will that state have (e.g. can it hold Python objects? If it can, then how do we deal with reference counting and traversing it?)

These aren't unsolvable problems, but I think the approach would be much easier if we just allowed passing a Python object, which has answered all of these questions. If the user really wants some C data, they can use a capsule.

@picnixz picnixz added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 23, 2024
@rhansen
Copy link
Contributor Author

rhansen commented Nov 24, 2024

I can do PyObject * instead of void *. I suggested void * because that's what I'm used to with pure C and thought it might be easier for developers connecting C code to Python. I was unaware of capsules; thanks for mentioning them.

Spitballing: Instead of a function pointer + object, WDYT about taking a callable object? That would be more awkward to use from C, unless there's a utility function I'm unaware of that wraps a C function pointer inside a callable.

@ZeroIntensity
Copy link
Member

ZeroIntensity commented Nov 24, 2024

That might be better, it would unlock some more use cases. You can use PyCFunction_New to wrap a C function into a Python callable, but yeah it's slightly more painful to use.

rhansen added a commit to rhansen/cpython that referenced this issue Nov 24, 2024
This enables users to associate state with the callback without
relying on globals.

Also:
  * Refactor the tests for improved readability and extensibility, and
    to cover the new state object.
  * Drop the pointer from the `PyContext_WatchCallback` typedef.  This
    de-obfuscates the fact that pointers are involved, and makes it
    possible to forward-declare functions to improve readability:

        static PyContext_WatchCallback my_callback;

        int
        my_callback(PyObject *cbarg, PyContextEvent event, PyObject *obj)
        {
            ...
        }
rhansen added a commit to rhansen/cpython that referenced this issue Nov 24, 2024
This enables users to associate state with the callback without
relying on globals.

Also:
  * Refactor the tests for improved readability and extensibility, and
    to cover the new state object.
  * Drop the pointer from the `PyContext_WatchCallback` typedef.  This
    de-obfuscates the fact that pointers are involved, and makes it
    possible to forward-declare functions to improve readability:

        static PyContext_WatchCallback my_callback;

        int
        my_callback(PyObject *cbarg, PyContextEvent event, PyObject *obj)
        {
            ...
        }
rhansen added a commit to rhansen/cpython that referenced this issue Nov 25, 2024
This enables developers to associate state with the callback without
relying on globals.

Also, refactor the tests for improved readability and extensibility,
and to cover the new state object.
rhansen added a commit to rhansen/cpython that referenced this issue Nov 26, 2024
rhansen added a commit to rhansen/cpython that referenced this issue Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants