Make sure chip-all-clusters-app is ready before we try to do PASE setup with it. #7561
Conversation
bzbarsky-apple
force-pushed
the
better-waiting-for-server
branch
from
bb8ab03 to
460486e
Compare
pullapprove
bot
requested review from
andy31415,
chrisdecenzo,
Damian-Nordic,
hawk248 and
jepenven-silabs
|
@Damian-Nordic @andy31415 @jepenven-silabs @mspang could you take a look please? |
bzbarsky-apple
force-pushed
the
better-waiting-for-server
branch
5 times, most recently
from
f8fb35c to
988fa34
Compare
|
@msandstedt @woody-apple @mrjerryjohns Note that I just updated the PR with a change to exactly how background_pid is read: now we read it out of the file after the server has started up, to avoid reading it before the write to the file has actually happened. |
bzbarsky-apple
force-pushed
the
better-waiting-for-server
branch
from
988fa34 to
46c5b8c
Compare
…up with it. We were racing startup of chip-tool against that of chip-all-clusters-app, and if the former started faster it would send the first PASE handshake message before the latter was ready. Then it would wait 5 seconds before resending, which slowed the test down quite a bit
bzbarsky-apple
force-pushed
the
better-waiting-for-server
branch
from
46c5b8c to
385c8ea
Compare
| # Clear out our temp files so we don't accidentally do a stale | ||
| # read from them before we write to them. | ||
| rm -rf /tmp/all-clusters-log | ||
| rm -rf /tmp/pid |
There was a problem hiding this comment.
Should this do a 'sync' or a 'flush' to ensure the deletion actually happened?
There was a problem hiding this comment.
This does not seem relevant in that we don't care what the on-disk state is, just what happens when we open the filename, and that part should be all set once rm returns. In particular, I was running into issues where we were seeing a "Server Listening" still in the all-clusters-log from a previous iteration once I moved reading that to before reading /tmp/pid..... But if we rm that should not happen, as far as I know.
| while ! grep -q "Server Listening" /tmp/all-clusters-log; do | ||
| : | ||
| done | ||
| # Now read $background_pid from /tmp/pid; presumably it's |
There was a problem hiding this comment.
The term 'presumably' doesn't inspire confidence...
There was a problem hiding this comment.
No, it does not! We are racing the I/O redirection to /tmp/pid in the background shell against the all-clusters-app/tee combo that is writing to /tmp/all-clusters-log. I would expect the former to win the race and put the PID in the right place before we see "Server Listening" in pretty much all cases, but there's no guarantee here. :(
We need to figure out a better setup for this....
…up with it. (project-chip#7561) We were racing startup of chip-tool against that of chip-all-clusters-app, and if the former started faster it would send the first PASE handshake message before the latter was ready. Then it would wait 5 seconds before resending, which slowed the test down quite a bit
We were racing startup of chip-tool against that of
chip-all-clusters-app, and if the former started faster it would send
the first PASE handshake message before the latter was ready. Then it
would wait 5 seconds before resending, which slowed the test down
quite a bit
Problem
chip-tool would try to talk to chip-all-clusters-app before chip-all-clusters-app was ready.
Change overview
Wait until chip-all-clusters-app logs that it's waiting for a PASE handshake before talking to it.
Testing
Manual testing of the script locally, and CI runs.