Avoid busy looping in case of send failure when probing.

Avoid busy looping in case of send failure while probing.

BUG=webrtc:7255
Review-Url: https://codereview.webrtc.org/2719183004
Cr-Commit-Position: refs/heads/master@{#16908}
Committed: https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f3899fb1239458e969

[philipel, 2017-02-28 14:10:04]

philipel@webrtc.org changed reviewers:
+ stefan@webrtc.org, tommi@chromium.org

[philipel, 2017-02-28 14:10:58]

Description was changed from

==========
Avoid busy looping in case of send failure when probing.

BUG=webrtc:7255
==========

to

==========
Avoid busy looping in case of send failure when probing.

BUG=webrtc:7255
==========

[philipel, 2017-02-28 14:10:58]

philipel@webrtc.org changed reviewers:
+ tommi@webrtc.org
- tommi@chromium.org

[philipel, 2017-02-28 14:11:16]

Description was changed from

==========
Avoid busy looping in case of send failure when probing.

BUG=webrtc:7255
==========

to

==========
Avoid busy looping in case of send failure while probing.

BUG=webrtc:7255
==========

[stefan-webrtc, 2017-02-28 14:23:41]

https://codereview.webrtc.org/2719183004/diff/1/webrtc/modules/pacing/paced_s...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2719183004/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:453: probing_send_failure_ = bytes_sent ==
0;
Why do we fail to send?

[tommi, 2017-02-28 14:30:44]

lgtm

[philipel, 2017-02-28 14:30:59]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2017-02-28 14:31:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-28 15:05:23]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1488292259009350, "parent_rev":
"13f54b2c5684119a1b31f172b3745cbec7229ebb", "commit_rev":
"b61927c6879a21cd9b8085f3899fb1239458e969"}

[commit-bot: I haz the power, 2017-02-28 15:05:25]

Description was changed from

==========
Avoid busy looping in case of send failure while probing.

BUG=webrtc:7255
==========

to

==========
Avoid busy looping in case of send failure while probing.

BUG=webrtc:7255

Review-Url: https://codereview.webrtc.org/2719183004
Cr-Commit-Position: refs/heads/master@{#16908}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...
==========

[commit-bot: I haz the power, 2017-02-28 15:05:26]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...

[mflodman, 2017-02-28 18:14:31]

On 2017/02/28 15:05:26, commit-bot: I haz the power wrote:
> Committed patchset #1 (id:1) as
>
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...

Reiterating Stefan's question,
Do we know why we get send errors for the probe? If not, I'd like a bug filed
for that to make sure we follow up.

[philipel, 2017-03-01 09:23:53]

On 2017/02/28 18:14:31, mflodman wrote:
> On 2017/02/28 15:05:26, commit-bot: I haz the power wrote:
> > Committed patchset #1 (id:1) as
> >
>
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...
> 
> Reiterating Stefan's question,
> Do we know why we get send errors for the probe? If not, I'd like a bug filed
> for that to make sure we follow up.

I am not sure if we ever fail to send while the network is up, but assuming that
it can happen it would cause a busy loop. I have not seen an example where we
actually fail to send.

[tommi, 2017-03-01 09:52:47]

On 2017/03/01 09:23:53, philipel wrote:
> On 2017/02/28 18:14:31, mflodman wrote:
> > On 2017/02/28 15:05:26, commit-bot: I haz the power wrote:
> > > Committed patchset #1 (id:1) as
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...
> > 
> > Reiterating Stefan's question,
> > Do we know why we get send errors for the probe? If not, I'd like a bug
filed
> > for that to make sure we follow up.
> 
> I am not sure if we ever fail to send while the network is up, but assuming
that
> it can happen it would cause a busy loop. I have not seen an example where we
> actually fail to send.

Would it make sense to have histograms to report when it happens? (and possibly
an error code too)

[mflodman, 2017-03-01 16:53:05]

On 2017/03/01 09:52:47, tommi (webrtc) wrote:
> On 2017/03/01 09:23:53, philipel wrote:
> > On 2017/02/28 18:14:31, mflodman wrote:
> > > On 2017/02/28 15:05:26, commit-bot: I haz the power wrote:
> > > > Committed patchset #1 (id:1) as
> > > >
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...
> > > 
> > > Reiterating Stefan's question,
> > > Do we know why we get send errors for the probe? If not, I'd like a bug
> filed
> > > for that to make sure we follow up.
> > 
> > I am not sure if we ever fail to send while the network is up, but assuming
> that
> > it can happen it would cause a busy loop. I have not seen an example where
we
> > actually fail to send.

Ok, my understanding was that this was a real issue caused by send failures.

> 
> Would it make sense to have histograms to report when it happens? (and
possibly
> an error code too)

Yes, something like that would probably be good.

Stefan,
What do you think?

[mflodman, 2017-03-01 16:53:08]

On 2017/03/01 09:52:47, tommi (webrtc) wrote:
> On 2017/03/01 09:23:53, philipel wrote:
> > On 2017/02/28 18:14:31, mflodman wrote:
> > > On 2017/02/28 15:05:26, commit-bot: I haz the power wrote:
> > > > Committed patchset #1 (id:1) as
> > > >
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...
> > > 
> > > Reiterating Stefan's question,
> > > Do we know why we get send errors for the probe? If not, I'd like a bug
> filed
> > > for that to make sure we follow up.
> > 
> > I am not sure if we ever fail to send while the network is up, but assuming
> that
> > it can happen it would cause a busy loop. I have not seen an example where
we
> > actually fail to send.

Ok, my understanding was that this was a real issue caused by send failures.

> 
> Would it make sense to have histograms to report when it happens? (and
possibly
> an error code too)

Yes, something like that would probably be good.

Stefan,
What do you think?

[stefan-webrtc, 2017-03-01 16:56:01]

On 2017/03/01 16:53:08, mflodman wrote:
> On 2017/03/01 09:52:47, tommi (webrtc) wrote:
> > On 2017/03/01 09:23:53, philipel wrote:
> > > On 2017/02/28 18:14:31, mflodman wrote:
> > > > On 2017/02/28 15:05:26, commit-bot: I haz the power wrote:
> > > > > Committed patchset #1 (id:1) as
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...
> > > > 
> > > > Reiterating Stefan's question,
> > > > Do we know why we get send errors for the probe? If not, I'd like a bug
> > filed
> > > > for that to make sure we follow up.
> > > 
> > > I am not sure if we ever fail to send while the network is up, but
assuming
> > that
> > > it can happen it would cause a busy loop. I have not seen an example where
> we
> > > actually fail to send.
> 
> Ok, my understanding was that this was a real issue caused by send failures.
> 
> > 
> > Would it make sense to have histograms to report when it happens? (and
> possibly
> > an error code too)
> 
> Yes, something like that would probably be good.
> 
> Stefan,
> What do you think?

Can we instead try to remove the path where we failed to send? I have doubts of
whether it even can happen.

[tommi, 2017-03-09 19:07:58]

On 2017/03/01 16:56:01, stefan-webrtc wrote:
> On 2017/03/01 16:53:08, mflodman wrote:
> > On 2017/03/01 09:52:47, tommi (webrtc) wrote:
> > > On 2017/03/01 09:23:53, philipel wrote:
> > > > On 2017/02/28 18:14:31, mflodman wrote:
> > > > > On 2017/02/28 15:05:26, commit-bot: I haz the power wrote:
> > > > > > Committed patchset #1 (id:1) as
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...
> > > > > 
> > > > > Reiterating Stefan's question,
> > > > > Do we know why we get send errors for the probe? If not, I'd like a
bug
> > > filed
> > > > > for that to make sure we follow up.
> > > > 
> > > > I am not sure if we ever fail to send while the network is up, but
> assuming
> > > that
> > > > it can happen it would cause a busy loop. I have not seen an example
where
> > we
> > > > actually fail to send.
> > 
> > Ok, my understanding was that this was a real issue caused by send failures.
> > 
> > > 
> > > Would it make sense to have histograms to report when it happens? (and
> > possibly
> > > an error code too)
> > 
> > Yes, something like that would probably be good.
> > 
> > Stefan,
> > What do you think?
> 
> Can we instead try to remove the path where we failed to send? I have doubts
of
> whether it even can happen.

My email didn't make it to the cr, so sending again in case we could just add a
CHECK() instead of being uncertain:

"What we know is that we didn't handle it and no data on whether or not it
actually happens. I'd prefer to know. If there's a way to know for sure, then
that would be good. If you're sure Stefan, we could just RTC_CHECK that it
doesn't and then we don't have to worry about error codes etc."

[stefan-webrtc, 2017-03-10 11:01:23]

On 2017/03/09 19:07:58, tommi (webrtc) wrote:
> On 2017/03/01 16:56:01, stefan-webrtc wrote:
> > On 2017/03/01 16:53:08, mflodman wrote:
> > > On 2017/03/01 09:52:47, tommi (webrtc) wrote:
> > > > On 2017/03/01 09:23:53, philipel wrote:
> > > > > On 2017/02/28 18:14:31, mflodman wrote:
> > > > > > On 2017/02/28 15:05:26, commit-bot: I haz the power wrote:
> > > > > > > Committed patchset #1 (id:1) as
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/b61927c6879a21cd9b8085f38...
> > > > > > 
> > > > > > Reiterating Stefan's question,
> > > > > > Do we know why we get send errors for the probe? If not, I'd like a
> bug
> > > > filed
> > > > > > for that to make sure we follow up.
> > > > > 
> > > > > I am not sure if we ever fail to send while the network is up, but
> > assuming
> > > > that
> > > > > it can happen it would cause a busy loop. I have not seen an example
> where
> > > we
> > > > > actually fail to send.
> > > 
> > > Ok, my understanding was that this was a real issue caused by send
failures.
> > > 
> > > > 
> > > > Would it make sense to have histograms to report when it happens? (and
> > > possibly
> > > > an error code too)
> > > 
> > > Yes, something like that would probably be good.
> > > 
> > > Stefan,
> > > What do you think?
> > 
> > Can we instead try to remove the path where we failed to send? I have doubts
> of
> > whether it even can happen.
> 
> My email didn't make it to the cr, so sending again in case we could just add
a
> CHECK() instead of being uncertain:
> 
> "What we know is that we didn't handle it and no data on whether or not it
> actually happens. I'd prefer to know. If there's a way to know for sure, then
> that would be good. If you're sure Stefan, we could just RTC_CHECK that it
> doesn't and then we don't have to worry about error codes etc."

I'm not sure, but given how our sockets are implemented I believe there should
be no errors propagated back up through return values. I'll see if I can add a
CHECK.