Remove ports that are not used by any channel after timeout

Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.


BUG=
R=pthatcher@webrtc.org, zhihuang@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/a74363c9982a080b6d35a68b43f2b57d4116532d

[honghaiz3, 2016-07-22 06:13:30]

Description was changed from

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.

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

to

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.

Also merged two signals: SignalPortPruned and SignalPortsRemoved.

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

[honghaiz3, 2016-07-22 15:59:00]

Description was changed from

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.

Also merged two signals: SignalPortPruned and SignalPortsRemoved.

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

to

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.


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

[honghaiz3, 2016-07-22 18:33:55]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-07-22 18:34:05]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2016-07-22 18:34:13]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2016-07-22 18:35:08]

honghaiz@webrtc.org changed reviewers:
+ andresp@webrtc.org, pthatcher@webrtc.org, zhihuang@webrtc.org

[Zhi Huang, 2016-07-26 03:18:13]

On 2016/07/22 18:35:09, honghaiz3 wrote:

lgtm

[pthatcher1, 2016-07-27 18:33:16]

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:155: }
for (PortAllocatorSession session : allocator_sessions_) {
  session->PruneAllPorts();
}

Would seem good.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:157: ports_.clear();
Can we move this into a method called RemoveAllPorts and put it next to
RemovePort?

Better yet, call them PrunePort and PruneAllPorts :).

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:476: port->StartBeingUsed();
Like we mentioned in person, I think this could either be done by the session or
just by the port itself when it has a candidate ready.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1752: port->StopBeingUsed();
Like we talked about in person, having this be Port::Prune and perhaps be called
by Session would make sense.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:130: // Note: They are only for testing
purpose.
They are => These are 
or => The following are

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:3620:
EXPECT_EQ_SIMULATED_WAIT(nullptr, GetRemovedPort(&ch), 1, fake_clock);
If we move the logic to be between the session and port, we'll need to move this
unit test.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:157: virtual void StopBeingUsed();
As we talked in person, would it make sense to just use the Prune() method.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:403: bool is_used_ = false;
Maybe we can just use !pruned_ && !candidates_.empty() here?

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:404: int64_t last_time_all_connections_removed_ = 0;
Can you move this "fix" for the timing of deletion into a separate CL?

[honghaiz3, 2016-07-28 01:21:12]

Patchset #3 (id:100001) has been deleted

[honghaiz3, 2016-07-28 01:21:21]

Patchset #3 (id:120001) has been deleted

[honghaiz3, 2016-07-28 01:22:49]

PTAL. 

Now that I am done, I realize that if we do it this way, we have to do the same
on all PortAllocatorSession implementation in order to prune ports everywhere. 
Not sure if that is a big issue.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:155: }
On 2016/07/27 18:33:15, pthatcher1 wrote:
> for (PortAllocatorSession session : allocator_sessions_) {
>   session->PruneAllPorts();
> }
> 
> Would seem good.

Done. Only need to do the last one.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:157: ports_.clear();
On 2016/07/27 18:33:15, pthatcher1 wrote:
> Can we move this into a method called RemoveAllPorts and put it next to
> RemovePort?
> 
> Better yet, call them PrunePort and PruneAllPorts :).
You suggested a different name in another pending CL. 
"RemovePort =>OnPortPruned"
I am merging the change from that one but taking the name PrunePort.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:476: port->StartBeingUsed();
On 2016/07/27 18:33:15, pthatcher1 wrote:
> Like we mentioned in person, I think this could either be done by the session
or
> just by the port itself when it has a candidate ready.

Done.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1752: port->StopBeingUsed();
On 2016/07/27 18:33:15, pthatcher1 wrote:
> Like we talked about in person, having this be Port::Prune and perhaps be
called
> by Session would make sense.

Done.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:130: // Note: They are only for testing
purpose.
On 2016/07/27 18:33:15, pthatcher1 wrote:
> They are => These are 
> or => The following are 

Done.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:3620:
EXPECT_EQ_SIMULATED_WAIT(nullptr, GetRemovedPort(&ch), 1, fake_clock);
On 2016/07/27 18:33:15, pthatcher1 wrote:
> If we move the logic to be between the session and port, we'll need to move
this
> unit test.

This is an extra test here. There are tests in Port.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:157: virtual void StopBeingUsed();
On 2016/07/27 18:33:15, pthatcher1 wrote:
> As we talked in person, would it make sense to just use the Prune() method.  

Done.

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:403: bool is_used_ = false;
On 2016/07/27 18:33:15, pthatcher1 wrote:
> Maybe we can just use !pruned_ && !candidates_.empty() here?

I think you said better to have state. Removed "is_used_"

https://codereview.webrtc.org/2171183002/diff/80001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:404: int64_t last_time_all_connections_removed_ = 0;
On 2016/07/27 18:33:15, pthatcher1 wrote:
> Can you move this "fix" for the timing of deletion into a separate CL?

Done.

[honghaiz3, 2016-07-28 01:22:50]

PTAL. 

Now that I am done, I realize that if we do it this way, we have to do the same
on all PortAllocatorSession implementation in order to prune ports everywhere. 
Not sure if that is a big issue.

[pthatcher1, 2016-07-28 19:35:06]

Mostly style/comment stuff.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:649: // If it is pruned, we won't bring it up again.
We should probably RTC_DCHECK(state_ == State::INIT).

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:662: if (dead()) {
Since there is only one place calling dead() now, I suggest we just inline it.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:668: return state_ != State::READY &&
connections_.empty();
I think (state_ == INIT || state_ == PRUNED) && connections_.empty() would be
more readable, and more careful to adding more states later.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:125: enum class State { INIT, READY, PRUNED };
This could use a description of states.  Such as:

This needs a better description.  Perhaps something "A pruned port or a port in
the pruned state will be destroyed as soon as no candidate pairs are using it
for a period of 30 seconds".  A live-until-pruned port will not be destroyed
until it is pruned.  Pruning a port moves it to PRUNED.  No other state
transitions are allowed.

By the way, I think "ALIVE_UNTIL_PRUNED" is a better name than "ready".

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:158: void BecomeReady();
I think a better name would be "KeepAliveUntilPruned", which keeps it alive
until it's pruned.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:159: // Called when a port is pruned.
A better comment would be "Allows the port to be destroyed once no connections
are using it."

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:306: enum { MSG_CHECK_DEAD = 0, MSG_FIRST_AVAILABLE };
MSG_MAYBE_DESTORY might be a better name than MSG_CHECK_DEAD.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/portallo...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/portallo...
webrtc/p2p/base/portallocator.h:193: // destroyed if there is no connection on
them.
if there is no connection on them => if no connections are using them.

[honghaiz3, 2016-07-28 22:50:50]

Patchset #4 (id:160001) has been deleted

[honghaiz3, 2016-07-28 22:50:58]

Patchset #4 (id:180001) has been deleted

[honghaiz3, 2016-07-28 22:51:05]

Patchset #4 (id:200001) has been deleted

[honghaiz3, 2016-07-28 22:51:44]

honghaiz@webrtc.org changed reviewers:
- andresp@webrtc.org

[honghaiz3, 2016-07-28 22:51:45]

PTAL. Addressed comments.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:649: // If it is pruned, we won't bring it up again.
On 2016/07/28 19:35:05, pthatcher1 wrote:
> We should probably RTC_DCHECK(state_ == State::INIT).
It may happen that it was pruned first then it become ready, because we don't
stop a session when ICE restart (if it has not been started). 
We could change that behavior, but we should keep it at a separate CL.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:662: if (dead()) {
On 2016/07/28 19:35:05, pthatcher1 wrote:
> Since there is only one place calling dead() now, I suggest we just inline it.
 

Done.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:668: return state_ != State::READY &&
connections_.empty();
On 2016/07/28 19:35:05, pthatcher1 wrote:
> I think (state_ == INIT || state_ == PRUNED) && connections_.empty() would be
> more readable, and more careful to adding more states later.

Done.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:125: enum class State { INIT, READY, PRUNED };
On 2016/07/28 19:35:05, pthatcher1 wrote:
> This could use a description of states.  Such as:
> 
> This needs a better description.  Perhaps something "A pruned port or a port
in
> the pruned state will be destroyed as soon as no candidate pairs are using it
> for a period of 30 seconds".  A live-until-pruned port will not be destroyed
> until it is pruned.  Pruning a port moves it to PRUNED.  No other state
> transitions are allowed.
> 
> By the way, I think "ALIVE_UNTIL_PRUNED" is a better name than "ready".

Done.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:158: void BecomeReady();
On 2016/07/28 19:35:05, pthatcher1 wrote:
> I think a better name would be "KeepAliveUntilPruned", which keeps it alive
> until it's pruned.

Done.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:306: enum { MSG_CHECK_DEAD = 0, MSG_FIRST_AVAILABLE };
On 2016/07/28 19:35:05, pthatcher1 wrote:
> MSG_MAYBE_DESTORY might be a better name than MSG_CHECK_DEAD.

Using MSG_DESTROY_IF_DEAD.

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/portallo...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/portallo...
webrtc/p2p/base/portallocator.h:193: // destroyed if there is no connection on
them.
On 2016/07/28 19:35:05, pthatcher1 wrote:
> if there is no connection on them => if no connections are using them.

Done.

[honghaiz3, 2016-07-28 22:52:28]

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2171183002/diff/140001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:159: // Called when a port is pruned.
On 2016/07/28 19:35:05, pthatcher1 wrote:
> A better comment would be "Allows the port to be destroyed once no connections
> are using it."

Done.

[pthatcher1, 2016-07-29 00:38:00]

lgtm

https://codereview.webrtc.org/2171183002/diff/220001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2171183002/diff/220001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:126: // KEEP_ALIVE_UNTIL_RRUNED: A port should not be
destroyed even if no
RRUNED => PRUNED

[honghaiz3, 2016-07-29 00:41:23]

https://codereview.webrtc.org/2171183002/diff/220001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2171183002/diff/220001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:126: // KEEP_ALIVE_UNTIL_RRUNED: A port should not be
destroyed even if no
On 2016/07/29 00:38:00, pthatcher1 wrote:
> RRUNED => PRUNED

Done.

[honghaiz3, 2016-07-29 00:56:20]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-07-29 00:56:20]

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

[commit-bot: I haz the power, 2016-07-29 00:56:25]

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, 2016-07-29 01:06:30]

Description was changed from

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.


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

to

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.

BUG=
R=pthatcher@webrtc.org, zhihuang@webrtc.org

Committed: https://crrev.com/a74363c9982a080b6d35a68b43f2b57d4116532d
Cr-Commit-Position: refs/heads/master@{#13567}
==========

[honghaiz3, 2016-07-29 01:06:30]

Description was changed from

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.


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

to

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.


BUG=
R=pthatcher@webrtc.org, zhihuang@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/a74363c9982a080b6d35a68b4...
==========

[commit-bot: I haz the power, 2016-07-29 01:06:31]

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

[honghaiz3, 2016-07-29 01:06:32]

Committed patchset #5 (id:240001) manually as
a74363c9982a080b6d35a68b43f2b57d4116532d (presubmit successful).

[juberti1, 2016-08-26 00:47:32]

On 2016/07/29 01:06:32, honghaiz3 wrote:
> Committed patchset #5 (id:240001) manually as
> a74363c9982a080b6d35a68b43f2b57d4116532d (presubmit successful).

Does this only affect ports that had a connection which was was destroyed?

i.e. if we have created ports and are waiting for a connection from the remote
side for several minutes (e.g. AppRTC), will that still work?

[juberti1, 2016-08-26 00:47:50]

Description was changed from

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.


BUG=
R=pthatcher@webrtc.org, zhihuang@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/a74363c9982a080b6d35a68b4...
==========

to

==========
Remove ports that are not used by any channel after timeout
If a port is not used by any channel and if it has no connection for 30
seconds, it will be removed.
Note, as long as a port is used by a transport channel, it will be kept
even if it does not have any connection. This will be beneficial to
continual gathering because new connections can be created in the future
when network changes.


BUG=
R=pthatcher@webrtc.org, zhihuang@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/a74363c9982a080b6d35a68b4...
==========

[honghaiz3, 2016-08-29 19:08:43]

On 2016/08/26 00:47:32, juberti1 wrote:
> On 2016/07/29 01:06:32, honghaiz3 wrote:
> > Committed patchset #5 (id:240001) manually as
> > a74363c9982a080b6d35a68b43f2b57d4116532d (presubmit successful).
> 
> Does this only affect ports that had a connection which was was destroyed?
> 
> i.e. if we have created ports and are waiting for a connection from the remote
> side for several minutes (e.g. AppRTC), will that still work?

If we have created ports and are waiting for a connection from the remote side,
it will not be destroyed because it is still being "used" by the transport
channel (i.e., it is not pruned). 

If a port is pruned, even if it had never created connections, it will be
destroyed.