| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
… by dunder methods
|
Alternative approach: use some flag in PyStructObject struct to forbid mutation during pack(). |
Sorry, something went wrong.
Would it be possible to always disallow mutation? Raise an exception if __init__() is called twice. |
Sorry, something went wrong.
|
Why there is __init__() method at all? The Struct expected to be an immutable type. Maybe all logic in Struct___init__ (tp_init) should be moved to s_new (tp_new)? I think this will fix issue. Looking in the history, the custom __init__() was re-introduced back in #112358. Here is the plan:
|
Sorry, something went wrong.
There was a problem hiding this comment.
This is too complicated and expensive.
Would not it be easier to forbid calling __init__() more than once? This can break some weird code, so as an intermediate solution we can add a mutex flag which forbids calling __init__() when the object is used.
Sorry, something went wrong.
|
Seen #143382 (comment) after trying to review the code. I agree with @skirpichev, a flag is the right temporary solution, and we should think about getting rid of __init__() in future. |
Sorry, something went wrong.
Now modification of the Struct() while packing trigger a RuntimeError
|
Ok, new version just disallows mutation of Struct() when s_pack_internal() is running. When support for __init__() will be dropped - the mutex field can be removed. Edit: see #143643 for next steps. |
Sorry, something went wrong.
There was a problem hiding this comment.
Would not it prevent concurrent use of pack()? It would be undesirable. We need a counter (atomic for GIL-less build) which would allow concurrent operations, but block __init__(). Please test how it affects performance.
To be absolutely safe, we would need also a mutex which would block packing while __init__() is executed. Because there is a race condition between checking if it is safe to modify the struct state and modifying it. But it is very unlikely to happen in real world (why would anybody call __init__() concurrently with pack()?), so we can ignore this for now. Well, ignoring this issue until we forbid repeated calls of __init__() is also solution.
Sorry, something went wrong.
Hmm, indeed.
We can do this after a deprecation (see ongoing work in #143643). But then this will be a non-issue anymore. Then, maybe we can close the #143379 as a duplicate of #78724? The real problem here is that Struct()'s aren't immutable. |
Sorry, something went wrong.
|
Ok, this lacks mutex in __init__() and benchmarks. But I'll not push things further. Thanks for review. |
Sorry, something went wrong.
| PyBytesWriter_Discard(writer); | ||
| return NULL; | ||
| } | ||
| FT_ATOMIC_STORE_SSIZE(soself->mutex_cnt, prev_cnt); |
There was a problem hiding this comment.
Would it work to use FT_ATOMIC_ADD_SSIZE(soself->mutex_cnt, -1); instead?
And then add maybe assert(soself->mutex_cnt >= 0);.
Sorry, something went wrong.
|
|
||
| if (FT_ATOMIC_LOAD_SSIZE(self->mutex_cnt)) { | ||
| PyErr_SetString(PyExc_RuntimeError, | ||
| "Call Struct.__init__() in struct.pack()"); |
There was a problem hiding this comment.
| "Call Struct.__init__() in struct.pack()"); | |
| "cannot call Struct.__init__() in struct.pack()"); |
Sorry, something went wrong.
|
Oh, you closed your PR. Why? |
Sorry, something went wrong.
See issue thread. I think that this not solves some practical problem, but introduce code complexity. If we make eventually make Struct immutable - #143379 will be fixed. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Added a mutex flag to trigger RuntimeError if Struct() modification happens during packing.