372388 - False failures when waiting for selfserv.

Status: RESOLVED FIXED

(NSS :: Test, defect, P2)

Description

[Slavomir Katuscak]

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).

Comment 1

[Slavomir Katuscak]

Attachment: Fix for bug.

Comment 2

[Neil Williams]

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.

Comment 3

[Slavomir Katuscak]

Attachment: Fix v2.

Applied Neil's suggestion from comment 2.

Comment 4

[Alexei Volkov]

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.

Comment 5

[Slavomir Katuscak]

Alexei, you mean to call is_selfserv_alive at the beginning of wait_for_selfserv ?

Comment 6

[Alexei Volkov]

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.

Comment 7

[Neil Williams]

Slavo, shall I wait for another patch before reviewing?

Comment 8

[Slavomir Katuscak]

Attachment: 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 9

[Neil Williams]

Comment on attachment 259336 [details] [diff] [review]
Fix v3.

The messages look better too.

Comment 10

[Alexei Volkov]

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.

Comment 11

[Slavomir Katuscak]

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.

Comment 12

[Slavomir Katuscak]

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: 1.61.2.11; previous revision: 1.61.2.10
done