Remove deprecated unbounded queue #2925
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2925 +/- ##
================================================
Coverage 100.00000% 100.00000%
================================================
Files 124 122 -2
Lines 18427 18313 -114
Branches 1215 1206 -9
================================================
- Hits 18427 18313 -114
|
Also make the generation script always use unix newlines, even on Windows
| # waitqueue), but in the future we ever start support task priorities or fair | ||
| # scheduling |
There was a problem hiding this comment.
The grammar here is a bit odd. I would change it to something like
"but in the future if we ever support task priorities or
fair scheduling it should be fairly simple."
| @@ -43,7 +43,7 @@ def current_kqueue() -> select.kqueue: | |||
|
|
|||
| def monitor_kevent( | |||
| ident: int, filter: int | |||
| ) -> ContextManager[_core.UnboundedQueue[select.kevent]]: | |||
| ) -> ContextManager[MemoryReceiveChannel[select.kevent]]: | |||
There was a problem hiding this comment.
Huh actually I'm just realizing this is potentially a compat break. I can work around this using a kwarg but I need to see if this currently warns or not...
There was a problem hiding this comment.
Private subclass which adds the unbounded queue methods but warns if you call them?
graingert
force-pushed
the
eliminate-unbounded-queue
branch
from
ef73f38 to
009cbb5
Compare
| @@ -98,7 +99,7 @@ def process_events(self, events: EventResult) -> None: | |||
| if isinstance(receiver, _core.Task): | |||
| _core.reschedule(receiver, outcome.Value(event)) | |||
| else: | |||
| receiver.put_nowait(event) # TODO: test this line | |||
| receiver.send_nowait(event) # TODO: test this line | |||
There was a problem hiding this comment.
it's possible for this to fail if someone calls:
with trio.lowlevel.monitor_kevent() as q:
q.close()we should either catch the exception here or yield a clone
I'd add it to this PR for you but I just cannot work out how to cover this line - it should be easy but I'm new to kqueue so might be missing something
here's my attempt at a test - but it times out 009cbb5 (#2925)
There was a problem hiding this comment.
something like this:
from 9ddb5ecc47dda3a7502851967bb67247a99aefe5 mon sep 17 00:00:00 2001
from: thomas grainger <[email protected]>
date: tue, 24 dec 2024 08:24:41 +0000
subject: [patch] make sure closing monitor queues is a noop
---
src/trio/_core/_io_kqueue.py | 12 ++++++------
src/trio/_core/_io_windows.py | 13 +++++++------
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/src/trio/_core/_io_kqueue.py b/src/trio/_core/_io_kqueue.py
index fed4da83..08b5d966 100644
--- a/src/trio/_core/_io_kqueue.py
+++ b/src/trio/_core/_io_kqueue.py
@@ -137,12 +137,12 @@ class kqueueiomanager:
"attempt to register multiple listeners for same ident/filter pair",
)
send, recv = open_memory_channel[select.kevent](math.inf)
- self._registered[key] = send
- try:
- yield recv
- finally:
- send.close()
- self._registered.pop(key, none)
+ with send, recv:
+ self._registered[key] = send
+ try:
+ yield recv.clone()
+ finally:
+ self._registered.pop(key, none)
@_public
async def wait_kevent(
diff --git a/src/trio/_core/_io_windows.py b/src/trio/_core/_io_windows.py
index 4676e6c5..824e91f5 100644
--- a/src/trio/_core/_io_windows.py
+++ b/src/trio/_core/_io_windows.py
@@ -1040,9 +1040,10 @@ class windowsiomanager:
key = next(self._completion_key_counter)
send, recv = open_memory_channel[object](math.inf)
- self._completion_key_queues[key] = send
- try:
- yield (key, recv)
- finally:
- send.close()
- del self._completion_key_queues[key]
+ with send, recv:
+ self._completion_key_queues[key] = send
+ try:
+ yield (key, recv.clone())
+ finally:
+ del self._completion_key_queues[key]
--
2.43.0
|
@graingert the reason this stalled (and the reason I think your latest changes don't quite work) is because we need to provide a layer that has both memory channel and unbounded queue apis. Alternatively, a boolean kwarg to switch over. Otherwise, we're breaking backwards compatibility. |
|
These apis were sketches barely documented and not suitable for production use, they also warned if they were used. I think it's acceptable to break them in this case. |
Very much based off of #937; fixes #2922; should be in the next 0.x.0 release so don't merge this yet!