1444160 - nsISocketTransport.setTimeout() logic handling broken

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Honza Bambas (:mayhemer)]

nsSocketTransportService::DoPollIteration calculates in pretty weird way the time a socket has not responded with any outflags.

problem is that for |s.mElapsedTime += uint16_t(pollInterval);| the pollInterval doesn't have the necessary granularity, it's just seconds.  and we often return from poll way sooner than after a second, so.. you know how it looks with the s.mElapsedTime member ;)  it's still zero :)

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

instead of adding time spent in poll I simply remember when the socket started to be expected an event - i.e. when we add it to the active poll list.  then instead of adding poll-interval time to mElapsed and comparing to the timeout value, I simply do if (now - epoch) > timeout => do timeout.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da6f0043eb6beb6921d1e3c2829873d3734c090c

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8957615 [details] [diff] [review]
v1

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

this is a good find..

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8957615 [details] [diff] [review]
v1

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

I couldn't find any logic issues.
The only nits are the unnecessary arguments to the new methods.

::: netwerk/base/nsSocketTransportService2.cpp
@@ +348,5 @@
>        PodMove(mPollList + newSocketIndex + 2, mPollList + newSocketIndex + 1,
>                mActiveCount - newSocketIndex);
>      }
> +
> +    sock->StartTimeout(PR_IntervalNow());

Is there a use case where we call StartTimeout with something other than now? Otherwise it shouldn't take any params, and we can call PR_IntervalNow in StartTimeout()

@@ +515,5 @@
>      if (mPollList[0].fd) {
>          mPollList[0].out_flags = 0;
>          pollList = mPollList;
>          pollCount = mActiveCount + 1;
> +        pollTimeout = pendingEvents ? PR_INTERVAL_NO_WAIT : PollTimeout(ts);

Same here. If we only pass "now" to PollTimeout, maybe it doesn't need an argument.

::: netwerk/base/nsSocketTransportService2.h
@@ +173,5 @@
> +    public:
> +        bool IsTimedOut(PRIntervalTime now) const;
> +        void StartTimeout(PRIntervalTime now);
> +        void StopTimeout();
> +        PRIntervalTime TimeoutIn(PRIntervalTime now) const;

If we never pass anything else than now() maybe we don't need the argument.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #3)
> If we never pass anything else than now() maybe we don't need the argument.

It used to use a cached value (similarly to Poll > PollTimeout > TimeoutIn arg passing) but later I removed that.  Yes, we don't need the arg any more.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1480b57708
Fix calculation of network socket timeout logic. r=valentin, mayhemer

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ba1480b57708