Merged
Conversation
…ang in threaded mode The select() method was filtering out ObjectPipe instances (like the sniffer's close_pipe) from its return value. This prevented the sniffer's stop mechanism from working correctly in threaded mode - when sniffer.stop() sent to close_pipe, the select() method would unblock but not return the close_pipe, so the sniffer loop couldn't detect the stop signal and had to rely on continue_sniff timing, causing hangs under load. The fix includes close_pipe (ObjectPipe) instances in the select return value, so the sniffer loop properly detects the stop signal via the 'if s is close_pipe: break' check. Added two new tests: - sr1 timeout with threaded=True (no response scenario) - sr1 timeout with threaded=True and background CAN traffic The new "ISOTPSoftSocket select returns control ObjectPipe" test directly verifies that ISOTPSoftSocket.select() passes through ready ObjectPipe instances (e.g. the sniffer's close_pipe). This test deterministically FAILS without the fix and PASSES with it. The integration tests (sr1 timeout with threaded=True) are kept for end-to-end coverage but the race window is too narrow on Linux with TestSocket to reliably trigger the bug. Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Ben Gardiner <ben.l.gardiner@gmail.com>
…ocket is garbage collected
… (slcan) interface introduce mulitple tests to confirm that all the combinations of filters, threading, slow/fast interfaces work with the isotpsoft socket in the particularly problematic case of a SF request yielding an MF respoonse. The new tests currently fail for slow (slcan) interfaces
Make this timeout scheduler a daemon thread. This should fix the python 3.13 tox failures on windows.
Contributor
Author
|
hi @polybassa please consider this PR as a replacement for #4929 since it is commits on-top-of it and completely resolves all issues mentioned in #4838 |
Contributor
|
Luckily we have a lot of unit tests including "stress" tests for the ISOTPSoftSocket. Crossing fingers that it will pass the pipelines! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4938 +/- ##
==========================================
+ Coverage 80.10% 80.64% +0.53%
==========================================
Files 370 370
Lines 91733 91976 +243
==========================================
+ Hits 73482 74172 +690
+ Misses 18251 17804 -447
🚀 New features to boost your workflow:
|
Contributor
Author
I think the macos failure is unrelated. And almost all the missing code coverage is error handling edge cases. |
Contributor
|
Looks great! |
polybassa
reviewed
Feb 27, 2026
BenGardiner
force-pushed
the
fix-slcan-w-isotpsoftsocket
branch
from
4519f30 to
9ae0471
Compare
Contributor
Author
|
hmmm ok those pipelines went waaaaay worse than in my test PR here BenGardiner#2 -- I'm looking into why |
…ke close and timeouts more robust
…en the internal state will do
BenGardiner
force-pushed
the
fix-slcan-w-isotpsoftsocket
branch
from
9ae0471 to
1cfc067
Compare
Contributor
Author
|
ok @polybassa . |
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
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.
( I worked on this one with copilot / Claude 4.6 and also some gemini-cli over in BenGardiner#2 )
In addition to the change introduced in #4929 , this PR also introduces testcases to trigger the isotp soft socket regression with sr1() timeout on an MF response to a SF request that I have been experiencing . And then it fixes it by:
Verification:
builds on top of #4929
fixes (completely) #4838