search

noahmorrison / chevron

A Python implementation of mustache
MIT License
505 stars 55 forks source link

chevron no longer thread-safe #36

Open gber opened 6 years ago

gber commented 6 years ago

Commit 3aa1e2b3d72e0d392f24cfc85b98eb61defae5db introduces a global dict for caching tokens which makes the render() function no longer safe to use from multiple threads. I suppose the obvious solution is to create a Renderer class which can hold the cache in order to avoid global state. The render() function could become a simple wrapper function for backwards compatibility.

noahmorrison commented 6 years ago

Sorry if I'm not understanding?

I don't like the global dictionary, so I'm gonna keep this open til I can fix it, but why exactly is it not thread-safe?

The items that it is inserting are constant per key, so even if two threads end up running the same render() function, whichever finishes first will place the right data into the cache, and then the slower one will just overwrite it.

Unless I'm wrong in which case I'll get rid of that cache as quickly as I can.

gber commented 6 years ago

Sorry if I'm not understanding?

I don't like the global dictionary, so I'm gonna keep this open til I can fix it, but why exactly is it not thread-safe?

The items that it is inserting are constant per key, so even if two threads end up running the same render() function, whichever finishes first will place the right data into the cache, and then the slower one will just overwrite it.

Maybe I should have formulated it more carefully, I am not a 100% certain but I don't think Python (the language, not the CPython implementation) makes any guarantees that replacing a dict entry is an atomic operation (though it might be in current CPython due to implementation details). And if an update is not atomic (but e.g. split into a remove and add operation), there would be a race condition between your check for the existence of the entry's key and subsequent access.

logankaser commented 5 years ago

For the builtin hashable types a dictionary update is a single bytecode instruction STORE_SUBSCR (in CPython a single instruction is implementation defined to be atomic) but this only applies to CPython and even then custom __hash__ methods make each update take several instructions. So likely works because of quirks in CPython, Other implementations are almost certainly going to have problems.