fix: use thread-safe iterator to generate context ids #716
Conversation
product-auto-label
bot
added
the
api: datastore
google-cla
bot
added
the
cla: yes
| self.lock = threading.Lock() | ||
|
|
||
| def __next__(self): | ||
| with self.lock: |
There was a problem hiding this comment.
I think there's a good chance itertools.count is threadsafe, but it doesn't seem to be officially documented as such, so I figured it was better not to assume that.
There was a problem hiding this comment.
By inspection, the C implementation doesn't make any calls which would release the GIL, AFAICT. @jimfulton do you have any other intuition about it?
|
Notably, I did write a regression test, using |
| self.lock = threading.Lock() | ||
|
|
||
| def __next__(self): | ||
| with self.lock: |
There was a problem hiding this comment.
By inspection, the C implementation doesn't make any calls which would release the GIL, AFAICT. @jimfulton do you have any other intuition about it?
|
|
||
|
|
||
| _context_ids = _generate_context_ids() | ||
| _context_ids = _ContextIds() |
There was a problem hiding this comment.
Are there no explicit tests for _generate_context_ids which need updating?
There was a problem hiding this comment.
There aren't, no. It was a private implementation detail of the constructor and got called in the majority of unit tests, so I didn't need it for coverage. I did go ahead and add some asserts for it in the test for the Context constructor. I also went ahead and added a non-deterministic regression test for this bug. I figure it's better than nothing.
Fixes #715