Add transaction to event slots #233
Conversation
More robust handling of mapping between origin and its hashed value
for more information, see https://pre-commit.ci
|
Thanks for the PR, I will have a look soon. |
| if getattr(self, "_origin_hash", None) is not None: | ||
| self._doc._origins.remove(cast(int, self._origin_hash)) |
There was a problem hiding this comment.
Why using getattr, since _origin_hash is set in __init__?
There was a problem hiding this comment.
I don't know why, but I got hit by missing _origin_hash in some cases. It may have to do with some inheritance pattern.
There was a problem hiding this comment.
I think that it's something to investigate, instead of using getattr because we don't know why it doesn't work without. Otherwise that's going to lead to code that we don't understand and don't know how to maintain.
| origin_hash = self._txn.origin() | ||
| if origin_hash is not None: | ||
| del self._doc._origins[origin_hash] |
There was a problem hiding this comment.
That was a very deterministic behavior, why did you move that part to the transaction's __del__ method?
There was a problem hiding this comment.
If you leave the logic there, then the tests won't work. Because when requesting the transaction origin from the event object the entry in the doc._origins will be gone.
The usage of __del__ is not less deterministic (at least in CPython) and it is more consistent as Transaction.origin will always be correct while the transaction object is reference somewhere.
There was a problem hiding this comment.
If you leave the logic there, then the tests won't work. Because when requesting the transaction origin from the event object the entry in the
doc._originswill be gone.
I think it's just a matter of dropping the transaction after removing the origin from the doc.
The usage of
__del__is not less deterministic
Yes it is, because now tests behave differently if running on CPython or PyPy.
|
@fcollonval Could you explain the reasoning about adding a reference count to origins? |
Co-authored-by: David Brochart <[email protected]>
There was a problem hiding this comment.
Thanks for the review @davidbrochart
Could you explain the reasoning about adding a reference count to origins?
If you create a helper model with methods like:
class MyDoc:
def __init__(self, ...):
self._doc = YDoc()
self._id = "my-unique-id"
def clear_things(self):
with self._doc.transaction(origin=self._id):
# Change the document
def update_things(self):
with self._doc.transaction(origin=self._id):
self.clear_things()
# Other document changesNot using a counter will result in discarding the origin from the mapping too early.
| origin_hash = self._txn.origin() | ||
| if origin_hash is not None: | ||
| del self._doc._origins[origin_hash] |
There was a problem hiding this comment.
If you leave the logic there, then the tests won't work. Because when requesting the transaction origin from the event object the entry in the doc._origins will be gone.
The usage of __del__ is not less deterministic (at least in CPython) and it is more consistent as Transaction.origin will always be correct while the transaction object is reference somewhere.
| if getattr(self, "_origin_hash", None) is not None: | ||
| self._doc._origins.remove(cast(int, self._origin_hash)) |
There was a problem hiding this comment.
I don't know why, but I got hit by missing _origin_hash in some cases. It may have to do with some inheritance pattern.
I don't think it's the case if the transaction is dropped after removing the origin from the doc. |
Fixes #232
Fixes #234
Add transaction to event slots.
This ends up a bit more complex to be able to keep a reference to the transaction in the event, this PR introduced a
BaseTransactionthat only keeps reference to the document and the origin (independently of the rust object).That ensures the introspection of the event at any time.
It also introduces a more robust way to maintain the mapping of Python origin and rust origin.