Issue with wait_for_selfserv() function in ssl.sh: 1. Tstclnt doesn't wait, so sometimes happened that selfserv is not already started and failure is reported. 2. If tstclnt can't connect selfserv, then it's tested once more, but results are not processed. Suggestion: 1. Wait 5 seconds before testing if tstclnt can connect to selfserv. 2. Remove second try (5 seconds should be enough).
Created attachment 257022 [details] [diff] [review] Fix for bug.
Comment on attachment 257022 [details] [diff] [review] Fix for bug. Instead of always waiting, how about: 1) try tstclnt, if failed wait 5 seconds, try tstclnt again.
Created attachment 257496 [details] [diff] [review] Fix v2. Applied Neil's suggestion from comment 2.
looks like the function wait_for_selfserv has reverse logic: I would check that process is alive using PID first, and then would go and use test client to access its port. If process with selfserv pid is present, we may want to try to connect more when just once after 5 sec(slow machine). If not, we should fail at once.
Alexei, you mean to call is_selfserv_alive at the beginning of wait_for_selfserv ?
yes, this seems most logical for me, to check if a selfserv process exists at all, and then try to connect to it, rather when do it other way around.
Slavo, shall I wait for another patch before reviewing?
Created attachment 259336 [details] [diff] [review] Fix v3. 1. Implemented ideas from comment 4. 2. Removed HTML message about positive check, because it's not regular tests, it's just check.
Comment on attachment 259336 [details] [diff] [review] Fix v3. The messages look better too.
Comment on attachment 259336 [details] [diff] [review] Fix v3. Patch is good. One thing though: I was wrong when proposed to move "is_selfserv_alive" to the beginning of the function. The problem is that we also need to verify that selfserv process with PID from SERVERPID file is alive after tstclnt had successfully connected to selfserv. If we do not check for this, we potentially can be connected to a selfserv process left from previous run. Please move "is_selfserv_alive" function call back to the old place before integrating the patch.
Yes, you're right. I'm moving is_selfserv_alive call to the end of wait_for_selfserv + removing message about PID from messages in wait_for_selfserv (we don't know PID in time when they are printed). Order of operations: 1. try to connect to selfserv (if can't then retry in 5 seconds) 2. check if file with PID exists 3. get PID 4. check if selfserv running (kill -0) Selfserv write it's PID to file before try to bind to port, so if there is running another selfserv instance on the same port, this test should detect it.
Trunk: Checking in ssl.sh; /cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v <-- ssl.sh new revision: 1.77; previous revision: 1.76 done Branch: Checking in ssl.sh; /cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v <-- ssl.sh new revision: 184.108.40.206; previous revision: 220.127.116.11 done