GH-141148: ensure tasks/threads get fresh copy of decimal.Context - #146482
GH-141148: ensure tasks/threads get fresh copy of decimal.Context#146482nascheme wants to merge 6 commits into
Conversation
Sorry, something went wrong.
Ensure that decimal.getcontext() returns a per-task copy of the decimal.Context() so that mutations are isolated between async tasks and threads using sys.flags.thread_inherit_context.
|
I think it's a correct approach. This introduce new C function, shouldn't it be discussed first with the C-API WG? |
Sorry, something went wrong.
Yes, I think so. We could make APIs private so that only the decimal module could use them. However, I think a pattern where you have contextvar values that you copy-on-update would be useful (the get_changed() method enables this). Otherwise, you have to "flatten" your data structure so that all of the bindings are inside the contextvars.Context() object. Or I suppose you can make those values immutable and re-bind a new one when you update. |
Sorry, something went wrong.
Yes, I suspect, that gmpy2 was affected as well: https://github.com/gmpy2/gmpy2/blob/194a6dba7c3f497eebe1b3ffa234addc15f2c7c1/src/gmpy2_context.c I think that same can be made without new API, using several contextvars.Context() objects. But I doubt about efficiency of such alternative.
I don't think it's possible without a huge compatibility break. |
Sorry, something went wrong.
| return 1; | ||
| } | ||
|
|
||
| return cur_val != orig_val; |
There was a problem hiding this comment.
Is the comparison by identity safe and correct here or should it call rich compare on the objects?
Sorry, something went wrong.
There was a problem hiding this comment.
I believe identity is what we want here.
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
Use a private API instead (ctx._get_changed() is the Python-level API).
Documentation build overview255 files changed · ± 248 modified · - 7 deleted ± Modified
- Deleted |
Sorry, something went wrong.
|
I revised this PR so it no longer adds a new public C API. I think there is little need to have get_changed() as a public API. That method is useful to make the decimal context more thread-safe. However, having a mutable object bound to the context variable is not a pattern we should encourage. Another concern about this is that it will keep the previous context vars alive for longer. For example, if a context variable is bound to a large object and then you re-bind it, the old object is keep alive via the ctx_vars_origin reference. Or, perhaps people expect a weakref callback or del method to run when the context var is re-bound. |
Sorry, something went wrong.
|
Switching this to a draft, since I feel like GH-151953 could be the better solution. |
Sorry, something went wrong.
Ensure that decimal.getcontext() returns a per-task copy of the decimal.Context so that mutations are isolated between asyncio tasks and threads when sys.flags.thread_inherit_context is set.
This change is required because decimal.Context instances are mutable. The contextvars.Context object uses an immutable data structure (HAMT) to store variable bindings. That ensures each task has its own set of contextvar bindings. However, it doesn't help if the contextvar value itself is mutated.
To fix this bug, the ContextVar.get_changed() method was added. This allows the decimal module to check if the contextvar value is newly set in this task/thread or if it is a value inherited. In the latter case, we copy the decimal.Context object to ensure that potential mutations to it are isolated.
There is a downside to this change. The contextvars.Context object has gained an additional pointer value PyHamtObject *ctx_vars_origin. I think the memory overhead of that pointer itself is negligible. That reference also keeps the entries in the "origin" HAMT object alive for as long as the new context exists. That could keep some contextvar values alive for longer.
I think in practice this is okay as well. Generally the contextvar values should be small objects. Also, if the task/thread that calls Context.run() is still around then the ctx_vars_origin object is likely still alive anyhow, due to that task holding a reference to it. It is only if the original task calls ContextVar.set() that you might free something.
📚 Documentation preview 📚: https://cpython-previews--146482.org.readthedocs.build/