gh-142414: Remove singleton design pattern in Lib/concurrent/interpreters/_crossinterp.py. #142577
Conversation
…est_interpreter_pool"
…THgP-.rst Co-authored-by: Peter Bierma <[email protected]>
| self.assertStartsWith(self.executor.submit(get_thread_name).result(), | ||
| "InterpreterPoolExecutor-"[:15]) | ||
|
|
||
| def test_cross_interpreter_unbound_identity(self): |
There was a problem hiding this comment.
FYI @ZeroIntensity This test will fail if we make the UNBOUND an object().
There was a problem hiding this comment.
What is this testing for? _queues.UNBOUND is not a public interface.
There was a problem hiding this comment.
It's defined under __all__, I guess it's okay to use?
__all__ = [
'UNBOUND', 'UNBOUND_ERROR', 'UNBOUND_REMOVE',
'create', 'list_all',
'Queue',
'QueueError', 'QueueNotFoundError', 'QueueEmpty', 'QueueFull',
'ItemInterpreterDestroyed',
]
This test verifies the object is consistent across interpreters.
There was a problem hiding this comment.
It's defined under __all__, but as far as I can tell, it's not exported to concurrent.interpreters, so the only way to access it is through the private API.
Generally speaking, I believe UNBOUND is supposed to be a sentinel that doesn't need a consistent state across interpreters. Do you have a case in mind where it would be useful to share UNBOUND?
There was a problem hiding this comment.
That's a good point.
If UNBOUND is sentinel, then we likely don't need this test, and can simplify singleton to object.
BTW, just in case you missed this, Lib/test/support/channels.py also exposes UNBOUND in all, but I guess it's also private in test, so we don't need to care that either.
There was a problem hiding this comment.
I just delete everything and make UNBOUND object. PTaL again!
This PR now becomes very simple, maybe we can make it part of #142515 (if we have no concerns to remove singleton.)
We don't need to make singleton picklabe based on the current tests.
note35
force-pushed
the
gh-142414-singleton-removal
branch
from
2b1d0f1 to
b128769
Compare
note35
force-pushed
the
gh-142414-singleton-removal
branch
from
c7cf990 to
f74dcf9
Compare
| except interpreters.QueueEmpty: | ||
| pass | ||
| except queues.QueueEmpty: | ||
| pass |
There was a problem hiding this comment.
This change is surprising. concurrent.interpreters.QueueEmpty is supposed to be concurrent.interpreters._queues.QueueEmpty.
There was a problem hiding this comment.
Yes, it's surprising. This PR is mostly about the singleton removal, It's probably okay to make all together now.
I provided some more context in #142515 (comment) .
(We can keep discussing here too if this PR is preferred.)
| @@ -0,0 +1 @@ | |||
| Fix spurious :exc:`KeyError` when :mod:`concurrent.interpreters` is reloaded after import. | |||
There was a problem hiding this comment.
| Fix spurious :exc:`KeyError` when :mod:`concurrent.interpreters` is reloaded after import. | |
| Fix spurious :exc:`KeyError` when :mod:`concurrent.interpreters._queues` is reloaded after import. |
3 participants
Test with
./python -m test test_interpreters test_struct./python -m test test_interpreters test_concurrent_futures.test_interpreter_pool./python -m test test_interpreters test_structfails with: KeyError: concurrent.interpreters._queues.UNBOUND #142414