Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Build] Test failed: src/messaging/tests/TestExchangeMgr.cpp:189: assertion failed: "!sendDelegate.IsOnResponseTimeoutCalled" '#3:','Test session eviction in timeout handling #27479

Closed
woody-apple opened this issue Jun 26, 2023 · 5 comments · Fixed by #27872
Closed

[Build] Test failed: src/messaging/tests/TestExchangeMgr.cpp:189: assertion failed: "!sendDelegate.IsOnResponseTimeoutCalled" '#3:','Test session eviction in timeout handling #27479

woody-apple opened this issue Jun 26, 2023 · 5 comments · Fixed by #27872
Assignees
@bzbarsky-apple @vivien-apple
Labels
build issue darwin
Milestone
1.2

Comments

@woody-apple
Copy link
Contributor

Build issue(s)

https://github.com/project-chip/connectedhomeip/actions/runs/5376188056/jobs/9753037388?pr=27470

[785/795] python3 ../../third_party/pigweed/repo/pw_build/py/pw_build/python_runner.py --gn-root ../../ --current-path ../../scripts/py_matter_yamltests --default-toolchain=//build/toolchain/host:mac_x64_gcc --current-toolchain=//third_party/pigweed/repo/pw_build/python_toolchain:python --touch python/gen/scripts/py_matter_yamltests/matter_yamltests.tests.test_yaml_loader.py.pw_pystamp --capture-output --python-virtualenv-config python/gen/matter_build_venv/venv_metadata.json --python-dep-list-files python/gen/scripts/py_matter_yamltests/matter_yamltests.tests.test_yaml_loader.py_metadata_path_list.txt -- ../../scripts/py_matter_yamltests/test_yaml_loader.py
[786/795] touch python/obj/scripts/py_matter_yamltests/matter_yamltests.tests.test_yaml_loader.py.stamp
[787/795] touch obj/scripts/py_matter_yamltests/matter_yamltests.tests.test_yaml_loader.py.stamp
[788/795] touch obj/scripts/py_matter_yamltests/matter_yamltests.tests.stamp
[789/795] python3 ../../third_party/pigweed/repo/pw_build/py/pw_build/python_runner.py --gn-root ../../ --current-path ../../src/transport/tests --default-toolchain=//build/toolchain/host:mac_x64_gcc --current-toolchain=//build/toolchain/host:mac_x64_gcc --touch gen/src/transport/tests/TestSessionManager_run.pw_pystamp --capture-output --module pw_unit_test.test_runner --python-virtualenv-config python/gen/matter_build_venv/venv_metadata.json --python-dep-list-files gen/src/transport/tests/TestSessionManager_run_metadata_path_list.txt -- --runner ../../third_party/pigweed/repo/targets/host/run_test --test tests/TestSessionManager
[790/795] touch obj/src/transport/tests/TestSessionManager_run.stamp
[791/795] touch obj/src/transport/tests/tests_run.stamp
ninja: build stopped: cannot make progress due to previous errors.
Error: Process completed with exit code 1.

Platform

No response

Anything else?

No response
@bzbarsky-apple
Copy link
Contributor 

src/messaging/tests/TestExchangeMgr.cpp:189: assertion failed: "!sendDelegate.IsOnResponseTimeoutCalled"

this has nothing to do with YAML or python.

@bzbarsky-apple
Copy link
Contributor

I am guessing we lost timeslice for > 100ms, so the timeout actually fired....

@woody-apple woody-apple changed the title [Build] pyyaml failed [Build] Build failed Jun 26, 2023
@woody-apple
Copy link
Contributor Author

Thanks, renaming!

@bzbarsky-apple
Copy link
Contributor

In particular:

[1687774193119] [15821:279924] [IN] SecureSession[0x600002e340f0]: Allocated for Test Type:1 LSID:2
'#3:','Test OnConnectionExpired timeout handling ','PASSED'
[1687774193258] [15821:279924] [EM] <<< [E:47080i S:2 M:40781666] (S) Msg TX to 1:0000000012344321 [6EF6] --- Type 0002:01 (BDX:SendInit)
[1687774193259] [15821:279924] [IN] (S) Sending msg 40781666 on secure session with LSID: 2

Note the 139ms time gap between previous test completion and us logging our send.... That's us losing the timeslice.

There's really no way to win here. We could bump the exchange timeout value to make this less likely, but of course then the test will always run slower.... and nothing guarantees that we won't lose the timeslice for a longer time either.

@woody-apple woody-apple changed the title [Build] Build failed [Build] Test failed: src/messaging/tests/TestExchangeMgr.cpp:189: assertion failed: "!sendDelegate.IsOnResponseTimeoutCalled" '#3:','Test session eviction in timeout handling Jul 7, 2023
@woody-apple
Copy link
Contributor Author

@bzbarsky-apple Let's up the timeout significantly here, it won't slow the test down that much compared to overall runtime.

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jul 10, 2023
@bzbarsky-apple
Fix exchange manager tests to handle losing timeslice better.
56c9863 
We could get into a situation where we lost the timeslice after the SendMessage
call and before we asserted the response timeout had not happened yet, which
would cause the test to fail.

The changes here are:

1) Move the assert that we are not timed out to _before_ SendMessage().  This
   ensures that our state is correct up front, and generally nothing under
   SendMessage proper or sending the message should trigger a timeout per se.
2) Use more slack when waiting for the timeout, just in case.

Fixes project-chip#27479
@bzbarsky-apple bzbarsky-apple mentioned this issue Jul 10, 2023
Merged
@bzbarsky-apple bzbarsky-apple moved this from Todo to In Progress in [Build] Build Issues Jul 10, 2023
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jul 10, 2023
@bzbarsky-apple
Fix exchange manager tests to handle losing timeslice better.
aa60040 
We could get into a situation where we lost the timeslice after the SendMessage
call and before we asserted the response timeout had not happened yet, which
would cause the test to fail.

The changes here are:

1) Move the assert that we are not timed out to _before_ SendMessage().  This
   ensures that our state is correct up front, and generally nothing under
   SendMessage proper or sending the message should trigger a timeout per se.
2) Use more slack when waiting for the timeout, just in case.

Fixes project-chip#27479
@woody-apple woody-apple added this to the 1.2 milestone Jul 11, 2023
@mergify mergify bot closed this as completed in #27872 Jul 11, 2023
mergify bot pushed a commit that referenced this issue Jul 11, 2023
@bzbarsky-apple
Fix exchange manager tests to handle losing timeslice better. (#27872)
e4a3b4b 
We could get into a situation where we lost the timeslice after the SendMessage
call and before we asserted the response timeout had not happened yet, which
would cause the test to fail.

The changes here are:

1) Move the assert that we are not timed out to _before_ SendMessage().  This
   ensures that our state is correct up front, and generally nothing under
   SendMessage proper or sending the message should trigger a timeout per se.
2) Use more slack when waiting for the timeout, just in case.

Fixes #27479
@github-project-automation github-project-automation bot moved this from Todo to Done in [Platform] Darwin Jul 11, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in [Build] Build Issues Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bzbarsky-apple bzbarsky-apple

@vivien-apple vivien-apple

Labels
build issue darwin
Projects
[Build] Build Issues
Archived in project
[Platform] Darwin
Archived in project
Milestone
1.2
Development

Successfully merging a pull request may close this issue.

Fix exchange manager tests to handle losing timeslice better. bzbarsky-apple/connectedhomeip
3 participants
@woody-apple @bzbarsky-apple @vivien-apple