If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

False failures when waiting for selfserv.

RESOLVED FIXED in 3.12

Status

NSS
Test
P2
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Slavomir Katuscak, Assigned: Slavomir Katuscak)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
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).
(Assignee)

Comment 1

11 years ago
Created attachment 257022 [details] [diff] [review]
Fix for bug.
Attachment #257022 - Flags: superreview?(neil.williams)
Attachment #257022 - Flags: review?(alexei.volkov.bugs)

Comment 2

11 years ago
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.
Attachment #257022 - Flags: superreview?(neil.williams) → superreview-
(Assignee)

Comment 3

11 years ago
Created attachment 257496 [details] [diff] [review]
Fix v2.

Applied Neil's suggestion from comment 2.
Attachment #257022 - Attachment is obsolete: true
Attachment #257496 - Flags: superreview?(neil.williams)
Attachment #257496 - Flags: review?(alexei.volkov.bugs)
Attachment #257022 - Flags: review?(alexei.volkov.bugs)

Comment 4

11 years ago
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.
(Assignee)

Comment 5

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

Comment 6

11 years ago
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

11 years ago
Slavo, shall I wait for another patch before reviewing?
(Assignee)

Comment 8

11 years ago
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.
Attachment #257496 - Attachment is obsolete: true
Attachment #259336 - Flags: superreview?(alexei.volkov.bugs)
Attachment #259336 - Flags: review?(neil.williams)
Attachment #257496 - Flags: superreview?(neil.williams)
Attachment #257496 - Flags: review?(alexei.volkov.bugs)

Comment 9

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

The messages look better too.
Attachment #259336 - Flags: review?(neil.williams) → review+

Comment 10

11 years ago
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.
Attachment #259336 - Flags: superreview?(alexei.volkov.bugs) → superreview+
(Assignee)

Comment 11

11 years ago
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.
(Assignee)

Comment 12

11 years ago
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
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Priority: -- → P2
Target Milestone: --- → 3.12
You need to log in before you can comment on or make changes to this bug.