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

PrefixList and PrefixMap are not thread-safe #1561

Closed
mdickinson opened this issue Sep 30, 2021 · 2 comments
Closed

PrefixList and PrefixMap are not thread-safe #1561

mdickinson opened this issue Sep 30, 2021 · 2 comments
Labels
resolution: not a bug The code is behaving as intended type: bug
Milestone

Comments

@mdickinson
Copy link
Member

mdickinson commented Sep 30, 2021

The PrefixList trait type has a hidden thread-safety trap, resulting from the mutable state stored on a PrefixList instance. Given

class A(HasTraits):
    foo = PrefixList(["no", "yes"])

two separate instances of A being used on different threads can interact via the values_ cache held on the PrefixList objects. For example, one thread might put a new element into the values_ cache while the other thread is attempting to iterate over it to find a value being looked up, leading to a "dictionary changed size during iteration" error.

A similar situation could occur with a single PrefixList instance shared across multiple classes:

YesNoPrefix = PrefixList(["no", "yes"])

class A(HasTraits):
     foo = YesNoPrefix

class B(HasTraits):
     bar = YesNoPrefix

We may want to consider using an LRU cache from the standard library, which has thread safety built in.

The PrefixMap class has a similar issue.

@mdickinson
Copy link
Member Author

I suspect that for common use-cases, the memoization is more trouble than it's worth; the simplest solution would be to remove it.

@mdickinson mdickinson added this to the 6.3.0 release milestone Sep 30, 2021
@mdickinson
Copy link
Member Author

For example, one thread might put a new element into the values_ cache while the other thread is attempting to iterate over it to find a value being looked up, leading to a "dictionary changed size during iteration" error.

This can't happen: we never iterate over the values_ cache. The only things we do with values_ are:

  • containment checks: (if value in self.values_)
  • lookups (return self.values_[value], having already established that the value is in self.values_)
  • assignments: self.values_[value] = match

There's a race condition in the containment check followed by the lookup, but since items are never deleted, the only failure mode there is that we can end up unnecessarily assigning a key to the dict that's already present.

Since self.values_ is a plain dict, and in current Python all of these operations are atomic, we're safe thanks to the GIL.

I still don't like this much, but it doesn't appear to be causing any actual problems right now.

@mdickinson mdickinson added the resolution: not a bug The code is behaving as intended label Oct 1, 2021
This was referenced Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution: not a bug The code is behaving as intended type: bug
Projects
None yet
Development

No branches or pull requests

1 participant