Closed Bug 925361 Opened 6 years ago Closed 6 years ago

Have TestStunServer try multiple ports

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: ekr, Assigned: bwc)

Details

Attachments

(1 file, 1 obsolete file)

I am seeing problems in the TestStunServer where it can't open a listen socket:

[==========] Running 36 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 15 tests from IceGatherTest
[ RUN      ] IceGatherTest.TestGatherFakeStunServerHostnameNoResolver
-332179008[7fffec122040]: Couldn't create listen socket

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffea2fd700 (LWP 2182)]
mozilla::TestStunServer::Reset (this=0x0)
    at /home/ekr/mozilla-inbound/media/mtransport/test/stunserver.cpp:423
423	  delay_ms_ = 0;

Most likely the problem is that the address/port is in use. The fix is to have
TestStunServer try multiple ports until it gets one it is happy with. There's
nothing special about 3478 here.
This problem occurs in automated tests on buildbot.
Byron, can you please do this today. This is causing problems on my automated build system.
Assignee: nobody → docfaraday
Also, please put a MOZ_ASSERT() in TestStunServer::GetInstance() to help diagnose future failures to create the TestStunServer()
Attachment #815460 - Flags: review?(ekr)
Comment on attachment 815460 [details] [diff] [review]
TestStunServer will retry opening its listen socket by incrementingthe port, to a maximum of 10 times.

Review of attachment 815460 [details] [diff] [review]:
-----------------------------------------------------------------

r- for a bunch of small issues.

::: media/mtransport/test/stunserver.cpp
@@ +205,5 @@
>    delete response_addr_;
>  }
>  
> +int TestStunServer::TryOpenListenSocket(nr_local_addr* addr, NR_SOCKET* fd) {
> +    int r = nr_transport_addr_set_port(&addr->addr, instance_port);

Isn't fd unused.

I would make instance_port an argument.

@@ +223,5 @@
> +      MOZ_MTLOG(ML_ERROR, "Couldn't create listen socket");
> +      return r;
> +    }
> +
> +    return 0;

These seem like different kinds of errors. The first two should never happen and the last one is the error of interest. We want to fail on the first two and retry on teh second.

I would return R_ALREADY for the last one and check for that below.

@@ +248,5 @@
>    }
>  
> +  NR_SOCKET fd;
> +  int tries = 10;
> +  while (tries-- != 0) {

tries--
Attachment #815460 - Flags: review?(ekr) → review-
(In reply to Eric Rescorla (:ekr) from comment #5)
> Comment on attachment 815460 [details] [diff] [review]
> TestStunServer will retry opening its listen socket by incrementingthe port,
> to a maximum of 10 times.
> 
> Review of attachment 815460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for a bunch of small issues.
> 
> ::: media/mtransport/test/stunserver.cpp
> @@ +205,5 @@
> >    delete response_addr_;
> >  }
> >  
> > +int TestStunServer::TryOpenListenSocket(nr_local_addr* addr, NR_SOCKET* fd) {
> > +    int r = nr_transport_addr_set_port(&addr->addr, instance_port);
> 
> Isn't fd unused.
> 
> I would make instance_port an argument.
>

fd is used by the caller. I'm fine with making instance_port an arg.
 
> @@ +223,5 @@
> > +      MOZ_MTLOG(ML_ERROR, "Couldn't create listen socket");
> > +      return r;
> > +    }
> > +
> > +    return 0;
> 
> These seem like different kinds of errors. The first two should never happen
> and the last one is the error of interest. We want to fail on the first two
> and retry on teh second.
> 
> I would return R_ALREADY for the last one and check for that below.

Makes sense.

> 
> @@ +248,5 @@
> >    }
> >  
> > +  NR_SOCKET fd;
> > +  int tries = 10;
> > +  while (tries-- != 0) {
> 
> tries--

Ok.
Attachment #815460 - Attachment is obsolete: true
Attachment #815498 - Flags: review?(ekr)
Comment on attachment 815498 [details] [diff] [review]
TestStunServer will retry opening its listen socket by incrementingthe port, to a maximum of 10 times.

Review of attachment 815498 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #815498 - Flags: review?(ekr) → review+
Comment on attachment 815498 [details] [diff] [review]
TestStunServer will retry opening its listen socket by incrementingthe port, to a maximum of 10 times.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b26717579eed
Attachment #815498 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/b26717579eed
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.