SystemLayerImplSelect: fix Request/ClearCallbackOnPendingXXX() for… #21135
Conversation
This file contains 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
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda,
msandstedt,
mspang and
saurabhst
|
PR #21135: Size comparison from e58147e to 3b3b1b1 Increases (2 builds for cc13x2_26x2, cyw30739)
Decreases (15 builds for cc13x2_26x2, linux, nrfconnect, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
@plan44 Could you look into the test failures, please? |
plan44
force-pushed
the
PR/fix_socket_event_watching
branch
from
3b3b1b1
to
683c97d
Compare
|
PR #21135: Size comparison from b647dfb to 683c97d Increases (3 builds for bl602, telink)
Decreases (3 builds for cyw30739, k32w, nrfconnect)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
…spatch, clean transport For CHIP_SYSTEM_CONFIG_USE_DISPATCH (Darwin) case, Request/ClearCallbackOnPendingXXX() were not working (no loop calling select to evaluate watches), but were called in TCP and UDP endpoint implementations, despite dispatch-specific extra code for the same purpose in the endpoints. This changeset now moves all CHIP_SYSTEM_CONFIG_USE_DISPATCH specific socket watching code into SystemLayerImplSelect, by making Request/ClearCallbackOnPendingXXX() actually working with dispatch. The issue surfaced in my (a bit exotic) use case when I needed to get callbacks for events on a plain socket not wrapped as TCPEndPoint. Doing this, I realized Request/ClearCallbackOnPendingXXX() were non-functional due to missing select main loop. Fixing this revealed the similar dispatch code in the endpoints that was needed to get those events handled despite Request/ClearCallbackOnPendingXXX() not working. That extra endpoint code now became redundant (delivering events twice), so had to be removed. I am aware that this might be a too deep intervention for coming from a CHIP newbie like me (not newbie in code deep diving, though), but I think it cleans up an inconsistency in the IP endpoints.
plan44
force-pushed
the
PR/fix_socket_event_watching
branch
from
683c97d
to
fd85659
Compare
|
PR #21135: Size comparison from b647dfb to fd85659 Increases (3 builds for bl602, telink)
Decreases (1 build for cyw30739)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
woody-apple
approved these changes
Jul 25, 2022
|
Fast tracking platform changes |
github-actions bot
pushed a commit
that referenced
this pull request
Jul 25, 2022
…spatch, clean transport (#21135) For CHIP_SYSTEM_CONFIG_USE_DISPATCH (Darwin) case, Request/ClearCallbackOnPendingXXX() were not working (no loop calling select to evaluate watches), but were called in TCP and UDP endpoint implementations, despite dispatch-specific extra code for the same purpose in the endpoints. This changeset now moves all CHIP_SYSTEM_CONFIG_USE_DISPATCH specific socket watching code into SystemLayerImplSelect, by making Request/ClearCallbackOnPendingXXX() actually working with dispatch. The issue surfaced in my (a bit exotic) use case when I needed to get callbacks for events on a plain socket not wrapped as TCPEndPoint. Doing this, I realized Request/ClearCallbackOnPendingXXX() were non-functional due to missing select main loop. Fixing this revealed the similar dispatch code in the endpoints that was needed to get those events handled despite Request/ClearCallbackOnPendingXXX() not working. That extra endpoint code now became redundant (delivering events twice), so had to be removed. I am aware that this might be a too deep intervention for coming from a CHIP newbie like me (not newbie in code deep diving, though), but I think it cleans up an inconsistency in the IP endpoints.
woody-apple
added a commit
that referenced
this pull request
Jul 25, 2022
…spatch, clean transport (#21135) (#21186) For CHIP_SYSTEM_CONFIG_USE_DISPATCH (Darwin) case, Request/ClearCallbackOnPendingXXX() were not working (no loop calling select to evaluate watches), but were called in TCP and UDP endpoint implementations, despite dispatch-specific extra code for the same purpose in the endpoints. This changeset now moves all CHIP_SYSTEM_CONFIG_USE_DISPATCH specific socket watching code into SystemLayerImplSelect, by making Request/ClearCallbackOnPendingXXX() actually working with dispatch. The issue surfaced in my (a bit exotic) use case when I needed to get callbacks for events on a plain socket not wrapped as TCPEndPoint. Doing this, I realized Request/ClearCallbackOnPendingXXX() were non-functional due to missing select main loop. Fixing this revealed the similar dispatch code in the endpoints that was needed to get those events handled despite Request/ClearCallbackOnPendingXXX() not working. That extra endpoint code now became redundant (delivering events twice), so had to be removed. I am aware that this might be a too deep intervention for coming from a CHIP newbie like me (not newbie in code deep diving, though), but I think it cleans up an inconsistency in the IP endpoints. Co-authored-by: Lukas Zeller <luz@plan44.ch>
| #if CHIP_SYSTEM_CONFIG_USE_DISPATCH | ||
| if (watch->mWrSource) | ||
| { | ||
| dispatch_resume(watch->mWrSource); |
There was a problem hiding this comment.
I don't think this is right... For example, TCP will call RequestCallbackOnPendingWrite for every single packet, without doing any ClearCallbackOnPendingWrite in between. So we will have multiple dispatch_resume calls when the dispatch source is already resumed. You're not allowed to do that, per the API documentation.
I have not carefully checked for other failure modes in TCP and UDP here, but fundamentally assuming that their calls to RequestCallback* and ClearCallback* will be ordered in a sane way is not reasonable: those calls have an API designed around a single boolean flag, not a counter. So we need to keep track with a boolean flag, and ensure our counter is only ever 0 or 1.
There was a problem hiding this comment.
I agree, I will update the change accordingly.
|
@plan44 Can you please fix the issue in #21135 (review) ? |
plan44
added a commit
to plan44/connectedhomeip
that referenced
this pull request
Jul 27, 2022
…learCallback As pointed out in project-chip#21135, the previous version was not safe when calls to RequestCallbackXX and ClearCallbackXX were not balanced, and there's no reason to believe API clients would balance them. This version does no longer try to suspend the source (because that could be problematic when done from within callbacks) but now follows the original semantics of just changing mPendingIO flags to enable/disable callback, with the exception of requesting the callback for the first time (which triggers creation of the dispatch source). WIP: more detailed testing shows: - TCPtest from src/transport/raw/tests never bothers to set up a dispatch queue in the first place, so fails. - when NetworkTestHelpers.cpp is modified to setup a dispatch queue, TCPtest succeeds, crashes or fails randomly - TestSystemWakeEvent from src/system/tests succeeds despite not setting up a dispatch queue. Apparently, the test result does not depend on the handlers set up actually returning events. - all other tests (run on macos, `ninja -C out/host check`) pass As making use of Request/ClearCallbackXXX without having a dispatch queue installed points to a problem (most likely only occurring in tests with only partial system init), I added a ChipLogError to make this visible.
|
@bzbarsky-apple Turns out to be much more difficult than I thought... I tried to fix and test the issue you pointed out. I tried several variants, but did not get TCPtest from src/transport/raw/tests producing reliable results (random crash/success/failure on every invocation). It does so in original form (where it does not bother to set up a dispatch queue at all) and also when I patched NetworkTestHelpers.cpp to install a dispatch queue. All other tests are fine (run on macOS). First I just made sure suspend/resume remains balanced in SystemLayerImplSelect as you suggested. This is as near as I could get to the original semantics where Request/ClearCallback just change flags - only the first RequestCallback installs the dispatch source. As I do not want to trigger the whole PR machinery with this WIP, this is on a separate branch in my fork for now: https://github.com/plan44/connectedhomeip/tree/PR/fix_socket_event_watching_WIP The failures in TCPtest seem more like a test artifact than a real problem to me, but I definitely don't understand the stack well enough yet to be sure. |
|
@plan44 When running the unit tests, there is no dispatch queue (long story). So the Basically, when there is no dispatch queue that code is supposed to fall back on the non-dispatch "just do sockets" code.... |
I can imagine ;-) Trying to debug I realized that the PlatformManager would be needed to properly set it up, firing up half of the stack for a test that should be simple.
Ok, thanks. That helped me to understand the problem: In my previous attempt I intentionally only set the flag when the source could actually be installed. This broke the select()-based DriveIO() fallback. The new version now just always sets the flag, and then tries to install the source, but when no dispatch is available it just outputs the warning. In non-test cases, the warning should definitely never appear, in tests it may, depending on the setup (and I changed the wording to make that clear). See new PR #21349 |
Yep, and then the rabbit hole gets deep. :) #21349 looks great, thank you! |
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
…spatch, clean transport (project-chip#21135) For CHIP_SYSTEM_CONFIG_USE_DISPATCH (Darwin) case, Request/ClearCallbackOnPendingXXX() were not working (no loop calling select to evaluate watches), but were called in TCP and UDP endpoint implementations, despite dispatch-specific extra code for the same purpose in the endpoints. This changeset now moves all CHIP_SYSTEM_CONFIG_USE_DISPATCH specific socket watching code into SystemLayerImplSelect, by making Request/ClearCallbackOnPendingXXX() actually working with dispatch. The issue surfaced in my (a bit exotic) use case when I needed to get callbacks for events on a plain socket not wrapped as TCPEndPoint. Doing this, I realized Request/ClearCallbackOnPendingXXX() were non-functional due to missing select main loop. Fixing this revealed the similar dispatch code in the endpoints that was needed to get those events handled despite Request/ClearCallbackOnPendingXXX() not working. That extra endpoint code now became redundant (delivering events twice), so had to be removed. I am aware that this might be a too deep intervention for coming from a CHIP newbie like me (not newbie in code deep diving, though), but I think it cleans up an inconsistency in the IP endpoints.
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.
…dispatch case, clean transport
Problem
For CHIP_SYSTEM_CONFIG_USE_DISPATCH (Darwin) case, Request/ClearCallbackOnPendingXXX() were not working (no loop calling select to evaluate watches).
But still, these methods were called in TCP and UDP endpoint implementations, despite dispatch-specific extra code for the same purpose in the endpoints.
Change overview
This changeset now moves all CHIP_SYSTEM_CONFIG_USE_DISPATCH specific socket watching code into SystemLayerImplSelect, by making Request/ClearCallbackOnPendingXXX() actually working with dispatch.
The issue surfaced in my (a bit exotic) use case when I needed to get callbacks for events on a plain socket not wrapped as TCPEndPoint.
Doing this, I realized Request/ClearCallbackOnPendingXXX() were non-functional due to the missing select main loop.
Fixing this revealed the similar dispatch code in the endpoints that was needed to get those events handled despite Request/ClearCallbackOnPendingXXX() not working. That dispatch specific endpoint code now became redundant (delivering events twice), so had to be removed.
Testing
I am aware that this might be a too deep intervention and too limited testing for coming from a CHIP newbie like me (not newbie in code deep diving, though), but I think it cleans up an inconsistency in the IP endpoints, and maybe can be of use after review by CHIP experts...