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

Allow dominate to work in async contexts #187

Merged
merged 2 commits into from
Dec 24, 2023
Merged

Allow dominate to work in async contexts #187

merged 2 commits into from
Dec 24, 2023

Conversation

tlonny
Copy link
Contributor

@tlonny tlonny commented Dec 13, 2023

Hi everyone,

This PR uses Context vars to help distinguish between different async contexts. This prevents two concurrent document edits running in an event loop from bleeding into each other.

With this fix, I hope dominate can be safely used with async web frameworks like sanic etc.

Using ContextVars to allow dominate to work within async contexts. Added
unit tests to ensure code works as expected.
@apalala
Copy link

apalala commented Dec 14, 2023

My apologies for jumping in here!

The proposed changes seem reasonable and simple enough.

Perhaps itertools.count() could be used for the counter? Yet my preference for unique identifiers is to use uuid.uuid1().

Also it would be good to take the opportunity to use something with less chances of collisions than hash().

https://stackoverflow.com/questions/47601592/safest-way-to-generate-a-unique-hash-in-python

@tlonny
Copy link
Contributor Author

tlonny commented Dec 14, 2023

Hi @apalala - thanks for your reply!

Although I can't imagine a situation where we get an accidental collision with async_context_id_counter, I agree it can't hurt to eliminate the risk entirely by using an UUID instead. Can I ask why uuid1 vs uuid4?

My understanding was that a region of the uuid1 is taken up by the computer's MAC address (or a similar, static identifier) - for the purposes of avoiding collisions, this region will always be the same and thus is "wasted space" that could otherwise be used to produce more randomness and push the collision risk lower.

With respect to your last point on hash() - why do we even need hash in the first place? Can't we just pass the tuple as-is into the dictionary?

@apalala
Copy link

apalala commented Dec 14, 2023

HI @tlonny!

Any UUID is good because it will resolve race conditions in both async and threading.

Using hash() is not satisfactory. Python's hash() is allowed to produce collisions because it's intention is to produce fair hashing (dict, set, etc.) and not uniqueness.

Using the tuple is correct, but using a sha1 or sha3 is more efficient in comparisons.

My own preference would be to use the tuple because it's correct, it's good enough, and it makes any upcoming debugging easier.

@tlonny
Copy link
Contributor Author

tlonny commented Dec 14, 2023

Great,

So action items for me:

  • implement unique async ids using UUIDs
  • return the raw tuple vs. its hash.

I'll get these added to the PR v. soon

Thanks for the feedback mate!

 - Added .venv and .envrc to .gitignore (I use direnv and venv to keep
   my python environments isolated - I hope this is okay!)
 - Removed print statements I left in dom_tag during debugging
 - Replaced global incrementing int with UUID for contextvar ID
   generation - this zeroes the risk of race-hazards/collisions
 - _get_thread_context now returns a tuple vs. a hash of a tuple.
   Functionally not much changes - the underlying dictionary will still
   use the same hashing function but the only difference is that _if_
   there is a collision, the dictionary will still be able to return the
   correct element
@tlonny
Copy link
Contributor Author

tlonny commented Dec 14, 2023

@apalala - Changes implemented :-)

@coveralls
Copy link

Coverage Status

coverage: 100.0%. remained the same
when pulling 5746232 on tlonny:master
into 4a66756 on Knio:master.

@tlonny
Copy link
Contributor Author

tlonny commented Dec 22, 2023

@Knio - are we good to merge this bad boy? Happy Xmas BTW

@Knio Knio merged commit d72dca8 into Knio:master Dec 24, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants