Cease all future TURN requests when a TURN refresh request fails.

Cease all future TURN requests when a TURN refresh request fails for a given TURN port.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388
Committed: https://crrev.com/6b9ab9204b9e0891cb0be2595a0d7ada92945cea
Cr-Commit-Position: refs/heads/master@{#11152}

[honghaiz3, 2015-12-29 17:24:02]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-12-29 17:27:38]

Patchset #2 (id:40001) has been deleted

[honghaiz3, 2015-12-29 17:28:05]

Description was changed from

==========
Clear all requests when turn port fails.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=
==========

to

==========
Clear all requests when turn port fails.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388
==========

[honghaiz3, 2015-12-29 17:31:23]

honghaiz@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[honghaiz3, 2015-12-29 17:31:24]

Although I did not add tests, by calling Close() asynchronously, it fixed a
crash in TestRefreshRequestGetsErrorResponse caused by double-delete if Close
were called synchronously.

[honghaiz3, 2015-12-29 17:31:25]

Although I did not add tests, by calling Close() asynchronously, it fixed a
crash in TestRefreshRequestGetsErrorResponse caused by double-delete if Close
were called synchronously.

[pthatcher1, 2015-12-30 17:20:41]

It looks good, but can we write a unit test for it?

Also the CL description could be a little more clear, with something like:

Cease all future TURN requests when a TURN refresh request fails for a given
TURN port.

This fixes an assert error in Turnport::OnSendStunPacket

[honghaiz3, 2015-12-30 20:21:29]

Description was changed from

==========
Clear all requests when turn port fails.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388
==========

to

==========
Cease all future stun requests when turn port refresh request fails.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388
==========

[honghaiz3, 2015-12-30 20:22:44]

Description was changed from

==========
Cease all future stun requests when turn port refresh request fails.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388
==========

to

==========
Cease all future TURN requests when a TURN refresh request fails for a given
TURN port.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388
==========

[honghaiz3, 2015-12-30 21:26:43]

Patchset #2 (id:60001) has been deleted

[honghaiz3, 2015-12-30 21:35:28]

On 2015/12/30 17:20:41, pthatcher1 wrote:
> It looks good, but can we write a unit test for it?
> 
> Also the CL description could be a little more clear, with something like:
> 
> Cease all future TURN requests when a TURN refresh request fails for a given
> TURN port.
> 
> This fixes an assert error in Turnport::OnSendStunPacket
Updated the description.

It is pretty hard to test that all stun requests are cleared. 
In order to test that, we will need to create multiple requests but flush only
the refresh request, but that is hard because the requests are not stored in the
order of creation time.

In the first attempt of this CL, I just tried to call Close() directly but it
caused crashes on the test. 
With async calling, it fixed the crash on the tests.

[pthatcher1, 2015-12-30 22:52:13]

I think that a simple solution for the flushing problem in the unit test is to
either is to update FlushRequests to take a predicate function that decides
whether to flush it or not.  Something like this:

void StunRequestManager::Flush(Predicate<StunRequest*> should_flush) {
  for (const auto kv : requests_) {
    StunRequest* request = kv.second;
    if (should_flush(request) {
      thread_->Clear(request, MSG_STUN_SEND);
      thread_->Send(request, MSG_STUN_SEND, NULL);
    } 
  }
}


Since Flush is only used by turnport_unittest.cc, this should be fairly easy.

https://codereview.webrtc.org/1547373002/diff/80001/webrtc/p2p/base/turnport_...
File webrtc/p2p/base/turnport_unittest.cc (left):

https://codereview.webrtc.org/1547373002/diff/80001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:710:
EXPECT_TRUE(turn_port_->connections().empty());
Shouldn't we still check this?

[honghaiz3, 2015-12-31 03:22:10]

You remind me that we can flush only certain type of messages. So I just added a
msg type in the Flush function.  Although we can pass in Predicate for find_if
functions, I have not seen Predicate used anywhere in webrtc.

https://codereview.webrtc.org/1547373002/diff/80001/webrtc/p2p/base/turnport_...
File webrtc/p2p/base/turnport_unittest.cc (left):

https://codereview.webrtc.org/1547373002/diff/80001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:710:
EXPECT_TRUE(turn_port_->connections().empty());
On 2015/12/30 22:52:13, pthatcher1 wrote:
> Shouldn't we still check this?
It wasn't needed because we did not create connections earlier.
Now that we create connections, add the check back.

[pthatcher1, 2016-01-05 00:10:03]

lgtm, with the naming nits

https://codereview.webrtc.org/1547373002/diff/100001/webrtc/p2p/base/stunrequ...
File webrtc/p2p/base/stunrequest.h (right):

https://codereview.webrtc.org/1547373002/diff/100001/webrtc/p2p/base/stunrequ...
webrtc/p2p/base/stunrequest.h:37: // Only for testing.
Can you make a named constant for 0, like kAllRequests?

https://codereview.webrtc.org/1547373002/diff/100001/webrtc/p2p/base/turnport.h
File webrtc/p2p/base/turnport.h (right):

https://codereview.webrtc.org/1547373002/diff/100001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.h:145: bool RequestsEmpty() { return
request_manager_.empty(); }
How about name this HasRequests() (with the opposite meaning)?

[honghaiz3, 2016-01-05 04:47:39]

https://codereview.webrtc.org/1547373002/diff/100001/webrtc/p2p/base/stunrequ...
File webrtc/p2p/base/stunrequest.h (right):

https://codereview.webrtc.org/1547373002/diff/100001/webrtc/p2p/base/stunrequ...
webrtc/p2p/base/stunrequest.h:37: // Only for testing.
On 2016/01/05 00:10:03, pthatcher1 wrote:
> Can you make a named constant for 0, like kAllRequests?

Done.

https://codereview.webrtc.org/1547373002/diff/100001/webrtc/p2p/base/turnport.h
File webrtc/p2p/base/turnport.h (right):

https://codereview.webrtc.org/1547373002/diff/100001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.h:145: bool RequestsEmpty() { return
request_manager_.empty(); }
On 2016/01/05 00:10:03, pthatcher1 wrote:
> How about name this HasRequests() (with the opposite meaning)?

Done.

[honghaiz3, 2016-01-05 04:47:52]

The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-05 04:48:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1547373002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1547373002/120001

[commit-bot: I haz the power, 2016-01-05 06:26:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-05 06:26:45]

Dry run: This issue passed the CQ dry run.

[honghaiz3, 2016-01-05 17:04:47]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-01-05 17:04:47]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1547373002/#ps120001 (title:
" ")

[commit-bot: I haz the power, 2016-01-05 17:04:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1547373002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1547373002/120001

[commit-bot: I haz the power, 2016-01-05 17:06:19]

Description was changed from

==========
Cease all future TURN requests when a TURN refresh request fails for a given
TURN port.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388
==========

to

==========
Cease all future TURN requests when a TURN refresh request fails for a given
TURN port.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388
==========

[commit-bot: I haz the power, 2016-01-05 17:06:19]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-01-05 17:06:25]

Description was changed from

==========
Cease all future TURN requests when a TURN refresh request fails for a given
TURN port.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388
==========

to

==========
Cease all future TURN requests when a TURN refresh request fails for a given
TURN port.
This fixes an assert error in Turnport::OnSendStunPacket

BUG=webrtc:5388

Committed: https://crrev.com/6b9ab9204b9e0891cb0be2595a0d7ada92945cea
Cr-Commit-Position: refs/heads/master@{#11152}
==========

[commit-bot: I haz the power, 2016-01-05 17:06:26]

Patchset 4 (id:??) landed as
https://crrev.com/6b9ab9204b9e0891cb0be2595a0d7ada92945cea
Cr-Commit-Position: refs/heads/master@{#11152}