fix(interfaces): address read queue review feedback - #3965
Draft
EmilyRagan wants to merge 4 commits into
Draft
EmilyRagan wants to merge 4 commits into
EmilyRagan wants to merge 4 commits into
Conversation
The Ruby ReadQueue shared one @raw_read_cancel flag that start_read_queue_thread reset. A read thread that survived both the join and kill_thread would see the reset flag, do one more read on the new stream and lose that data. A late read_queue_pop could also drive the byte counters negative. Each thread is now cancelled by closing its own queue under the mutex, and the pop only decrements the counters while its queue is still current, matching the per-thread cancel event used by Python. Addresses PR #3902 review feedback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ReadQueue relied on every subclass remembering to call initialize_read_queue(). A custom subclass that forgot would only fail later with an AttributeError on _read_queue_condition. ReadQueue now initializes the queue in its own __init__, so subclasses only need to call super().__init__(). Addresses PR #3902 review feedback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With the read queue, received_time and the raw data time were set when data came off the queue, so with up to READ_QUEUE_MAX_SIZE backed up they could trail the real arrival time by however long the backlog took to drain. The read thread now queues the time of each read alongside the data. Interface#read applies it as the packet received_time (when a protocol hasn't set one) and read_interface_base uses it for the raw data time. This makes timestamps more accurate than inline reads while the queue is backed up. Inline reads (READ_QUEUE_MAX_SIZE 0) are unchanged. Applies to both Ruby and Python. Addresses PR #3902 review feedback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
READ_QUEUE_MAX_SIZE applies per queue and the TCP/IP server has one queue per client, so memory scales with the number of clients and all interfaces in a container share it. Add a sizing note with an example. Also document that disconnect waits up to 2 seconds for the read thread when a custom stream's disconnect doesn't unblock a pending read. In Ruby the thread is then killed with a warning; in Python it is left blocked until its read returns. Tell custom stream authors to wake pending reads. Addresses PR #3902 review feedback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## interface_reads_and_metrics #3965 +/- ##
===============================================================
- Coverage 80.29% 79.79% -0.50%
===============================================================
Files 903 903
Lines 68675 68424 -251
Branches 2645 2686 +41
===============================================================
- Hits 55141 54600 -541
- Misses 12867 13156 +289
- Partials 667 668 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Contributor
Author
|
@ryanmelt the python CI failure here is already addressed in main, so you should update your branch with main |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Follow-up to #3902 addressing its review comments, in four commits:
Interface#readuses it as the packetreceived_timewhen a protocol hasn't set one, andread_interface_baseuses it for the raw data time. Before this, both reflected dequeue time and could lag arrival by the full backlog. Ruby and Python.@raw_read_cancelflag with closing each thread's own queue under the mutex.read_queue_poponly decrements the counters while its queue is still current. This matches Python's per-threadcancel_eventand stops a thread that surviveskill_threadfrom reading the new stream.ReadQueue.__init__. The read queue is now set up inReadQueue.__init__. Subclasses no longer need to rememberinitialize_read_queue().disconnectdoesn't unblock a pendingread.Why it changed
Review feedback on #3902.
Not addressed here:
Testing strategy
bundle exec rspec spec/interfaces spec/utilities: 890 examples, 0 failures.pytest test/interfaces test/utilities test/microservices: 688 passed. One failure,test_raises_a_timeout_when_unable_to_connect, also fails on the base branch locally.🤖 Generated with Claude Code