Do not delete the turn port entry right away when the respective connection is deleted.

Do not delete the turn port entry right away when the respective connection is deleted.
BUG=webrtc:5120
Committed: https://crrev.com/e58fe8ef0e6d959f54adee3ed77764927d3845cc
Cr-Commit-Position: refs/heads/master@{#10641}

[honghaiz3, 2015-11-04 22:49:21]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-11-04 23:53:02]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2015-11-05 00:02:51]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2015-11-05 00:05:35]

Patchset #1 (id:60001) has been deleted

[honghaiz3, 2015-11-05 00:06:05]

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

[honghaiz3, 2015-11-05 00:08:05]

Description was changed from

==========
Do not delete the turn port entry when the respective connection is deleted.
BUG=
==========

to

==========
Do not delete the turn port entry when the respective connection is deleted.
BUG=webrtc:5120
==========

[honghaiz3, 2015-11-05 00:50:23]

Description was changed from

==========
Do not delete the turn port entry when the respective connection is deleted.
BUG=webrtc:5120
==========

to

==========
Do not delete the turn port entry right away when the respective connection is
deleted.
BUG=webrtc:5120
==========

[honghaiz3, 2015-11-09 18:16:32]

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

[commit-bot: I haz the power, 2015-11-09 18:16:39]

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

[commit-bot: I haz the power, 2015-11-09 20:16:57]

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

[commit-bot: I haz the power, 2015-11-09 20:16:57]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pthatcher1, 2015-11-11 01:08:41]

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:168: uint32_t msg_id_ = 0;
I think this would be more clear as delayed_destroy_msg_id_. 

And the comment would be more clear as "If set, this entry is schedule to be
destroyed with the following message ID.  The message ID can be used to cancel
the scheduled destruction.".

But even then, I'm not quite sure putting in on the TurnEntry is the right
option.  I think it might make more sense to have a table in the port of id =>
turn_entry.  Basically std::map<int, TurnEntry*>.  Then, check the map to see if
it's scheduled for destruction or not.  Then we don't need to complicated the
TurnEntry class.

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:906: TurnEntry* TurnPort::CreateEntryIfNeeded(const
rtc::SocketAddress& addr) {
I think it would make sense to call this GetOrCreateEntry.  We have similarly
named things throughout the code base.

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:913: thread()->Clear(&invoker_, entry->msg_id());
I feel like this logic should be in its own method, perhaps something like
CancelEntryDestruction(entry).  We could have a companion method
"ScheduleEntryDestruction(entry)".

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:936: TURN_PERMISSION_TIMEOUT, last_msg_id_);
I think this is where a method called "ScheduleEntryDestruction(entry,
TURN_PERMISSION_TIMEOUT)" would makes sense.

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

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:436:
rtc::Thread::Current()->ProcessMessages(500);
This could end up being flaky.  Could we use EXPECT_TRUE_WAIT here instead so it
waits long enough?  Same with the ProcessMessages(100) below.

[honghaiz3, 2015-11-11 23:02:36]

Patchset #2 (id:100001) has been deleted

[honghaiz3, 2015-11-11 23:02:55]

PTAL.

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:168: uint32_t msg_id_ = 0;
On 2015/11/11 01:08:41, pthatcher1 wrote:
> I think this would be more clear as delayed_destroy_msg_id_. 
> 
> And the comment would be more clear as "If set, this entry is schedule to be
> destroyed with the following message ID.  The message ID can be used to cancel
> the scheduled destruction.".
> 
> But even then, I'm not quite sure putting in on the TurnEntry is the right
> option.  I think it might make more sense to have a table in the port of id =>
> turn_entry.  Basically std::map<int, TurnEntry*>.  Then, check the map to see
if
> it's scheduled for destruction or not.  Then we don't need to complicated the
> TurnEntry class.
If we use the map, the map would have to be map<TurnEntry*, int> to map the
Entry to the message id (because later, we need to find the message id from the
Found TurnEntry). Using a field in the TurnEntry is like a more direct way of
mapping the turn entry to the message id without the complication of maps. Using
a map for this may have other complications. For example, we will need to
maintain the consistence of the map and the existing TurnEntries (if an entry is
destroyed, also need to delete the one in the map, etc).

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:906: TurnEntry* TurnPort::CreateEntryIfNeeded(const
rtc::SocketAddress& addr) {
On 2015/11/11 01:08:41, pthatcher1 wrote:
> I think it would make sense to call this GetOrCreateEntry.  We have similarly
> named things throughout the code base.

Done.

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:913: thread()->Clear(&invoker_, entry->msg_id());
On 2015/11/11 01:08:41, pthatcher1 wrote:
> I feel like this logic should be in its own method, perhaps something like
> CancelEntryDestruction(entry).  We could have a companion method
> "ScheduleEntryDestruction(entry)".

Done.

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:936: TURN_PERMISSION_TIMEOUT, last_msg_id_);
On 2015/11/11 01:08:41, pthatcher1 wrote:
> I think this is where a method called "ScheduleEntryDestruction(entry,
> TURN_PERMISSION_TIMEOUT)" would makes sense.

Done.

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

https://codereview.webrtc.org/1426673007/diff/80001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:436:
rtc::Thread::Current()->ProcessMessages(500);
On 2015/11/11 01:08:41, pthatcher1 wrote:
> This could end up being flaky.  Could we use EXPECT_TRUE_WAIT here instead so
it
> waits long enough?  Same with the ProcessMessages(100) below.
Done. Cann't use the EXPECT_TRUE_WAIT because we are not waiting for changes.

[pthatcher1, 2015-11-12 00:40:20]

https://codereview.webrtc.org/1426673007/diff/120001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1426673007/diff/120001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:146: }
As we discussed, let's try and make this scheduled_destruction_time_.

[honghaiz3, 2015-11-12 02:17:12]

Patchset #3 (id:140001) has been deleted

[honghaiz3, 2015-11-12 02:24:04]

https://codereview.webrtc.org/1426673007/diff/120001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1426673007/diff/120001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:146: }
On 2015/11/12 00:40:20, pthatcher1 wrote:
> As we discussed, let's try and make this scheduled_destruction_time_.

When I tried to implement that, I found it became very difficult for testing. 
For example, in my current code, I can flush the invoker to process the queued
events. 
But if we check the timestamp, it won't take effect unless I actually waited for
that long. 
So using a message id may be better.

In fact, after second thought, I don't think there is any problem of using a
dynamic message ID, even though none of the existing code is doing it the same
way. message Id has a scope of the handler. In our case, it is the invoker. 
There should be no problem to create dynamic message id and use it. 
We can discuss more tomorrow.

[pthatcher1, 2015-11-13 01:59:15]

https://codereview.webrtc.org/1426673007/diff/120001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1426673007/diff/120001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:146: }
I think we can get two birds with one stone here.  The two issues I have with
this CL are the dynamic message ID and the fact that we don't actually test the
timing.  I think we can fix both of these with one solution:  pass in a fake
clock into TurnPort.  We've been meaning to make a fake clock into unit tests
anyway to make them faster and less flaky.  This looks like a good fit.

We don't have a fake clock yet, but here's a CL that would add one:

https://webrtc-codereview.appspot.com/42599004/

I just never submitted it.



With a fake clock, we could do the following in the tests:

1.  Pass in the fake clock to TurnPort.
2.  Cause it to be scheduled for destruction
3.  Move the fake clock forward up to 1 second before the scheduled destruction
4.  Flush the thread events
5.  Verify that it wasn't destroyed
6.  Cause it to be scheduled for destruction again
7.  Move the fake clock forward up to 1 second after the scheduled destruction
8.  Flush the thread events
9.  Verify it was destroyed

I think that's a better test and better code and lays a groundwork for other
tests to use the fake clock. 

What do you think?



On 2015/11/12 02:24:04, honghaiz3 wrote:
> On 2015/11/12 00:40:20, pthatcher1 wrote:
> > As we discussed, let's try and make this scheduled_destruction_time_.
> 
> When I tried to implement that, I found it became very difficult for testing. 
> For example, in my current code, I can flush the invoker to process the queued
> events. 
> But if we check the timestamp, it won't take effect unless I actually waited
for
> that long. 
> So using a message id may be better.
> 
> In fact, after second thought, I don't think there is any problem of using a
> dynamic message ID, even though none of the existing code is doing it the same
> way. message Id has a scope of the handler. In our case, it is the invoker. 
> There should be no problem to create dynamic message id and use it. 
> We can discuss more tomorrow.

[honghaiz3, 2015-11-13 22:36:07]

Patchset #3 (id:160001) has been deleted

[honghaiz3, 2015-11-13 22:41:22]

Patchset #3 (id:180001) has been deleted

[honghaiz3, 2015-11-13 22:44:16]

https://codereview.webrtc.org/1426673007/diff/120001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1426673007/diff/120001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:146: }
Revised as we discussed. We can avoid checking the time by using the timestamp
as a unique ID. 
we can use the fake clock for other tests later.

[pthatcher1, 2015-11-14 00:30:16]

lgtm with style nits

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:910: TurnEntry* TurnPort::GetOrCreateEntry(const
rtc::SocketAddress& addr) {
The more I think about this name, the more it seems like CreateOrRefreshEntry
would make more sense.

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:916: CancelEntryDestruction(entry);
And can you leave a comment saying "The entry will automatically refresh itself
until it's destroyed."

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:928: void TurnPort::MaybeDestroyEntry(TurnEntry*
entry, uint32_t timestamp) {
Can we call this DestroyEntryItNotCancelled?

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:930: DestroyEntry(entry);
And then have an early return:

bool cancelled = timestamp != entry->destruction_timestamp();
if (!cancelled) {
  DestroyEntry(entry);
}

[honghaiz3, 2015-11-14 01:02:52]

Patchset #5 (id:240001) has been deleted

[honghaiz3, 2015-11-14 01:04:21]

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:910: TurnEntry* TurnPort::GetOrCreateEntry(const
rtc::SocketAddress& addr) {
On 2015/11/14 00:30:16, pthatcher1 wrote:
> The more I think about this name, the more it seems like CreateOrRefreshEntry
> would make more sense.

Done. Also changed the return value to void.

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:916: CancelEntryDestruction(entry);
On 2015/11/14 00:30:16, pthatcher1 wrote:
> And can you leave a comment saying "The entry will automatically refresh
itself
> until it's destroyed."

Done. With slight change.

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:928: void TurnPort::MaybeDestroyEntry(TurnEntry*
entry, uint32_t timestamp) {
On 2015/11/14 00:30:16, pthatcher1 wrote:
> Can we call this DestroyEntryItNotCancelled?

Done.

https://codereview.webrtc.org/1426673007/diff/220001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport.cc:930: DestroyEntry(entry);
On 2015/11/14 00:30:16, pthatcher1 wrote:
> And then have an early return:
> 
> bool cancelled = timestamp != entry->destruction_timestamp();
> if (!cancelled) {
>   DestroyEntry(entry);
> }

Done.

[honghaiz3, 2015-11-14 01:05:34]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2015-11-14 01:05:35]

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/1426673007/#ps260001 (title:
" ")

[commit-bot: I haz the power, 2015-11-14 01:05:38]

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

[commit-bot: I haz the power, 2015-11-14 01:54:47]

Committed patchset #5 (id:260001)

[commit-bot: I haz the power, 2015-11-14 01:54:53]

Patchset 5 (id:??) landed as
https://crrev.com/e58fe8ef0e6d959f54adee3ed77764927d3845cc
Cr-Commit-Position: refs/heads/master@{#10641}

[tommi, 2015-11-16 12:12:49]

A revert of this CL (patchset #5 id:260001) has been created in
https://codereview.webrtc.org/1449863002/ by tommi@webrtc.org.

The reason for reverting is: I have to revert this unfortunately because it adds
a dependency on AsyncInvoker, which is not included when building libjingle_nacl
in Chromium.
AsyncInvoker needs to first be added to the list of sources in Chromium..