Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1247573002:
Fix a Turn TCP port issue. (Closed)
DescriptionFix Turn TCP port issue.
Sometimes the port still try to send stun packet when the connection is disconnected,
causing an assertion error.
BUG=4859
Committed: https://crrev.com/b19eba3d4bbc70ece91d524e21e2e9d4253ff7a9
Cr-Commit-Position: refs/heads/master@{#9671}
Patch Set 1 : #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : Slight change in asynctcpsocket.cc #
Total comments: 20
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Messages
Total messages: 60 (24 generated)
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
Please take a look at the CL. Will add tests later.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247573002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Added tests.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
On 2015/07/23 00:58:05, honghaiz3 wrote: > Added tests. Added the change in asynctcpsocket.cc. It is still required because sometimes network changed but packets are still being sent before the socket state is changed. Because of this change, the change in turnport.cc is not necessary but it will help reduce the number of unnecessary error messages.
Patchset #3 (id:120001) has been deleted
Patchset #1 (id:20001) has been deleted
https://codereview.webrtc.org/1247573002/diff/140001/webrtc/base/asynctcpsock... File webrtc/base/asynctcpsocket.cc (right): https://codereview.webrtc.org/1247573002/diff/140001/webrtc/base/asynctcpsock... webrtc/base/asynctcpsocket.cc:131: if (addr != remote_address) { Can you please add a special case for "empty" that logs with LS_WARNING and gives a specific message? https://codereview.webrtc.org/1247573002/diff/140001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/1247573002/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:37: STATE_CONNECTED // Stun response is received. Can you call these DISCONNECTED, CONNECTING, and CONNECTED? DISCONNECTED: No socket available, Can't send any packets CONNECTING: Socket available, can sent STUN packets, waiting for allocation response CONNECTED: Socket available and allocation response received. Can send any packets.
Patchset #3 (id:160001) has been deleted
On 2015/07/28 18:23:04, pthatcher1 wrote: > https://codereview.webrtc.org/1247573002/diff/140001/webrtc/base/asynctcpsock... > File webrtc/base/asynctcpsocket.cc (right): > > https://codereview.webrtc.org/1247573002/diff/140001/webrtc/base/asynctcpsock... > webrtc/base/asynctcpsocket.cc:131: if (addr != remote_address) { > Can you please add a special case for "empty" that logs with LS_WARNING and > gives a specific message? > > https://codereview.webrtc.org/1247573002/diff/140001/webrtc/p2p/base/turnport.h > File webrtc/p2p/base/turnport.h (right): > > https://codereview.webrtc.org/1247573002/diff/140001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport.h:37: STATE_CONNECTED // Stun response is > received. > Can you call these DISCONNECTED, CONNECTING, and CONNECTED? > > DISCONNECTED: No socket available, Can't send any packets > CONNECTING: Socket available, can sent STUN packets, waiting for allocation > response > CONNECTED: Socket available and allocation response received. Can send any > packets. PTAL.
honghaiz@webrtc.org changed reviewers: + juberti@webrtc.org
lgtm, assuming you give the enum a prefix. I'll leave port_state_ vs. state_ up to you. https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:38: // can send any packets. Unfortunately, these need a prefix, like PS_DISCONNECT, PS_CONNECTING, PS_CONNECTED. https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:237: PortState port_state_; Just state_ would be fine.
Patchset #5 (id:220001) has been deleted
On 2015/07/28 22:06:14, pthatcher1 wrote: > lgtm, assuming you give the enum a prefix. > > I'll leave port_state_ vs. state_ up to you. > > https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.h > File webrtc/p2p/base/turnport.h (right): > > https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport.h:38: // can send any packets. > Unfortunately, these need a prefix, like PS_DISCONNECT, PS_CONNECTING, > PS_CONNECTED. > > https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport.h:237: PortState port_state_; > Just state_ would be fine. Done. Thanks!
I think it would be simpler if we just purged all in-flight requests when the connection is lost. But maybe I am overlooking something. https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:275: LOG(LS_ERROR) << "IP Address family does not match: " Address -> address https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:341: // TCP port becomes CONNECTING after the socket is connected, The port becomes connecting after the socket is connected... that seems confusing. https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { Could we just flush pending packets when the connection drops, to avoid this problem?
https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:341: // TCP port becomes CONNECTING after the socket is connected, On 2015/07/29 05:28:13, juberti1 wrote: > The port becomes connecting after the socket is connected... that seems > confusing. It's already like that in the existing code, where connected_ = false even after the socket is connected. It's because port connected != socket connected. It's a consequence of using the word "connected" for both the port and the socket, even though they have different meanings. The only way around it is to think of a different name than "connected" for a port that is ready to send. "ready"? https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { On 2015/07/29 05:28:13, juberti1 wrote: > Could we just flush pending packets when the connection drops, to avoid this > problem? We could add a StunRequestManager::RemoveAll that would then call Thread::Clear. And we could call request_manager_->RemoveAll() here. That sounds like a good approach.
Patchset #6 (id:260001) has been deleted
Plus, I put the Enum inside the class so that it has less chance to be used accidentally.
https://codereview.webrtc.org/1247573002/diff/140001/webrtc/base/asynctcpsock... File webrtc/base/asynctcpsocket.cc (right): https://codereview.webrtc.org/1247573002/diff/140001/webrtc/base/asynctcpsock... webrtc/base/asynctcpsocket.cc:131: if (addr != remote_address) { Done On 2015/07/28 18:23:04, pthatcher1 wrote: > Can you please add a special case for "empty" that logs with LS_WARNING and > gives a specific message? https://codereview.webrtc.org/1247573002/diff/140001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/1247573002/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:37: STATE_CONNECTED // Stun response is received. On 2015/07/28 18:23:04, pthatcher1 wrote: > Can you call these DISCONNECTED, CONNECTING, and CONNECTED? > > DISCONNECTED: No socket available, Can't send any packets > CONNECTING: Socket available, can sent STUN packets, waiting for allocation > response > CONNECTED: Socket available and allocation response received. Can send any > packets. Done. https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:275: LOG(LS_ERROR) << "IP Address family does not match: " On 2015/07/29 05:28:13, juberti1 wrote: > Address -> address Done. https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:341: // TCP port becomes CONNECTING after the socket is connected, On 2015/07/29 20:51:50, pthatcher1 wrote: > On 2015/07/29 05:28:13, juberti1 wrote: > > The port becomes connecting after the socket is connected... that seems > > confusing. > > It's already like that in the existing code, where connected_ = false even after > the socket is connected. It's because port connected != socket connected. > It's a consequence of using the word "connected" for both the port and the > socket, even though they have different meanings. The only way around it is to > think of a different name than "connected" for a port that is ready to send. > "ready"? If we change the name, we'd better change all three. For now I changed the comments to make it less confusing. Please let me know if we should change the enum names. https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { On 2015/07/29 20:51:50, pthatcher1 wrote: > On 2015/07/29 05:28:13, juberti1 wrote: > > Could we just flush pending packets when the connection drops, to avoid this > > problem? > > We could add a StunRequestManager::RemoveAll that would then call Thread::Clear. > And we could call request_manager_->RemoveAll() here. That sounds like a good > approach. Actually there is a request_manager_->Clear method there and I tried that, but that does not eliminate all requests. Some the requests are the permission requests which are generated when TurnEntry is created, which is created from Connection, which is created when a new candidate is received in the signaling path... https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:38: // can send any packets. On 2015/07/28 22:06:14, pthatcher1 wrote: > Unfortunately, these need a prefix, like PS_DISCONNECT, PS_CONNECTING, > PS_CONNECTED. Done. https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:237: PortState port_state_; On 2015/07/28 22:06:14, pthatcher1 wrote: > Just state_ would be fine. Done.
https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { On 2015/07/29 21:44:13, honghaiz3 wrote: > On 2015/07/29 20:51:50, pthatcher1 wrote: > > On 2015/07/29 05:28:13, juberti1 wrote: > > > Could we just flush pending packets when the connection drops, to avoid this > > > problem? > > > > We could add a StunRequestManager::RemoveAll that would then call > Thread::Clear. > > And we could call request_manager_->RemoveAll() here. That sounds like a > good > > approach. > Actually there is a request_manager_->Clear method there and I tried that, but > that does not eliminate all requests. Some the requests are the permission > requests which are generated when TurnEntry is created, which is created from > Connection, which is created when > a new candidate is received in the signaling path... But if RequestManager called thread_->Clear(), I think it would remove all of the messages.
On 2015/07/29 22:00:10, pthatcher1 wrote: > https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc > File webrtc/p2p/base/turnport.cc (right): > > https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { > On 2015/07/29 21:44:13, honghaiz3 wrote: > > On 2015/07/29 20:51:50, pthatcher1 wrote: > > > On 2015/07/29 05:28:13, juberti1 wrote: > > > > Could we just flush pending packets when the connection drops, to avoid > this > > > > problem? > > > > > > We could add a StunRequestManager::RemoveAll that would then call > > Thread::Clear. > > > And we could call request_manager_->RemoveAll() here. That sounds like a > > good > > > approach. > > Actually there is a request_manager_->Clear method there and I tried that, but > > that does not eliminate all requests. Some the requests are the permission > > requests which are generated when TurnEntry is created, which is created from > > Connection, which is created when > > a new candidate is received in the signaling path... > > But if RequestManager called thread_->Clear(), I think it would remove all of > the messages. It won't because they are not scheduled in the message queue (thread). They are freshly created and sent.
lgtm
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247573002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247573002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247573002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247573002/280001
https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { On 2015/07/29 22:00:10, pthatcher1 wrote: > On 2015/07/29 21:44:13, honghaiz3 wrote: > > On 2015/07/29 20:51:50, pthatcher1 wrote: > > > On 2015/07/29 05:28:13, juberti1 wrote: > > > > Could we just flush pending packets when the connection drops, to avoid > this > > > > problem? > > > > > > We could add a StunRequestManager::RemoveAll that would then call > > Thread::Clear. > > > And we could call request_manager_->RemoveAll() here. That sounds like a > > good > > > approach. > > Actually there is a request_manager_->Clear method there and I tried that, but > > that does not eliminate all requests. Some the requests are the permission > > requests which are generated when TurnEntry is created, which is created from > > Connection, which is created when > > a new candidate is received in the signaling path... > > But if RequestManager called thread_->Clear(), I think it would remove all of > the messages. I think Clear() + a check during permission creation would be the best approach.
The CQ bit was unchecked by honghaiz@webrtc.org
https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { On 2015/07/30 18:22:33, juberti1 wrote: > On 2015/07/29 22:00:10, pthatcher1 wrote: > > On 2015/07/29 21:44:13, honghaiz3 wrote: > > > On 2015/07/29 20:51:50, pthatcher1 wrote: > > > > On 2015/07/29 05:28:13, juberti1 wrote: > > > > > Could we just flush pending packets when the connection drops, to avoid > > this > > > > > problem? > > > > > > > > We could add a StunRequestManager::RemoveAll that would then call > > > Thread::Clear. > > > > And we could call request_manager_->RemoveAll() here. That sounds like a > > > good > > > > approach. > > > Actually there is a request_manager_->Clear method there and I tried that, > but > > > that does not eliminate all requests. Some the requests are the permission > > > requests which are generated when TurnEntry is created, which is created > from > > > Connection, which is created when > > > a new candidate is received in the signaling path... > > > > But if RequestManager called thread_->Clear(), I think it would remove all of > > the messages. > > I think Clear() + a check during permission creation would be the best approach. That means we will keep all the changes on the PortState and check the state at the time of creating permission instead of sending the packet here, right?
On 2015/07/30 18:38:27, honghaiz3 wrote: > https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc > File webrtc/p2p/base/turnport.cc (right): > > https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { > On 2015/07/30 18:22:33, juberti1 wrote: > > On 2015/07/29 22:00:10, pthatcher1 wrote: > > > On 2015/07/29 21:44:13, honghaiz3 wrote: > > > > On 2015/07/29 20:51:50, pthatcher1 wrote: > > > > > On 2015/07/29 05:28:13, juberti1 wrote: > > > > > > Could we just flush pending packets when the connection drops, to > avoid > > > this > > > > > > problem? > > > > > > > > > > We could add a StunRequestManager::RemoveAll that would then call > > > > Thread::Clear. > > > > > And we could call request_manager_->RemoveAll() here. That sounds like > a > > > > good > > > > > approach. > > > > Actually there is a request_manager_->Clear method there and I tried that, > > but > > > > that does not eliminate all requests. Some the requests are the permission > > > > requests which are generated when TurnEntry is created, which is created > > from > > > > Connection, which is created when > > > > a new candidate is received in the signaling path... > > > > > > But if RequestManager called thread_->Clear(), I think it would remove all > of > > > the messages. > > > > I think Clear() + a check during permission creation would be the best > approach. > That means we will keep all the changes on the PortState and check the state at > the time of creating permission instead of sending the packet here, right?
https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:341: // TCP port becomes CONNECTING after the socket is connected, On 2015/07/29 21:44:13, honghaiz3 wrote: > On 2015/07/29 20:51:50, pthatcher1 wrote: > > On 2015/07/29 05:28:13, juberti1 wrote: > > > The port becomes connecting after the socket is connected... that seems > > > confusing. > > > > It's already like that in the existing code, where connected_ = false even > after > > the socket is connected. It's because port connected != socket connected. > > It's a consequence of using the word "connected" for both the port and the > > socket, even though they have different meanings. The only way around it is > to > > think of a different name than "connected" for a port that is ready to send. > > "ready"? > If we change the name, we'd better change all three. > For now I changed the comments to make it less confusing. Please let me know if > we should change the enum names. how about STATE_CONNECTING (init), STATE_CONNECTED (ready to send allocate), STATE_READY (received allocate success), STATE_DISCONNECTED (TCP connection died) https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { On 2015/07/30 18:38:27, honghaiz3 wrote: > On 2015/07/30 18:22:33, juberti1 wrote: > > On 2015/07/29 22:00:10, pthatcher1 wrote: > > > On 2015/07/29 21:44:13, honghaiz3 wrote: > > > > On 2015/07/29 20:51:50, pthatcher1 wrote: > > > > > On 2015/07/29 05:28:13, juberti1 wrote: > > > > > > Could we just flush pending packets when the connection drops, to > avoid > > > this > > > > > > problem? > > > > > > > > > > We could add a StunRequestManager::RemoveAll that would then call > > > > Thread::Clear. > > > > > And we could call request_manager_->RemoveAll() here. That sounds like > a > > > > good > > > > > approach. > > > > Actually there is a request_manager_->Clear method there and I tried that, > > but > > > > that does not eliminate all requests. Some the requests are the permission > > > > requests which are generated when TurnEntry is created, which is created > > from > > > > Connection, which is created when > > > > a new candidate is received in the signaling path... > > > > > > But if RequestManager called thread_->Clear(), I think it would remove all > of > > > the messages. > > > > I think Clear() + a check during permission creation would be the best > approach. > That means we will keep all the changes on the PortState and check the state at > the time of creating permission instead of sending the packet here, right? right. We would ASSERT if we get a packet when STATE_DISCONNECTED. https://codereview.webrtc.org/1247573002/diff/240001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/1247573002/diff/240001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:34: enum PortState { This should be TurnPortState, since it is at global scope
https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:341: // TCP port becomes CONNECTING after the socket is connected, On 2015/07/30 20:47:40, juberti1 wrote: > On 2015/07/29 21:44:13, honghaiz3 wrote: > > On 2015/07/29 20:51:50, pthatcher1 wrote: > > > On 2015/07/29 05:28:13, juberti1 wrote: > > > > The port becomes connecting after the socket is connected... that seems > > > > confusing. > > > > > > It's already like that in the existing code, where connected_ = false even > > after > > > the socket is connected. It's because port connected != socket connected. > > > It's a consequence of using the word "connected" for both the port and the > > > socket, even though they have different meanings. The only way around it > is > > to > > > think of a different name than "connected" for a port that is ready to send. > > > > "ready"? > > If we change the name, we'd better change all three. > > For now I changed the comments to make it less confusing. Please let me know > if > > we should change the enum names. > > how about STATE_CONNECTING (init), STATE_CONNECTED (ready to send allocate), > STATE_READY (received allocate success), STATE_DISCONNECTED (TCP connection > died) That sounds fine. The only strange part is that connected() will be implemented with "state_ == STATE_READY", which is confusing.
https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:341: // TCP port becomes CONNECTING after the socket is connected, On 2015/07/30 21:29:16, pthatcher1 wrote: > On 2015/07/30 20:47:40, juberti1 wrote: > > On 2015/07/29 21:44:13, honghaiz3 wrote: > > > On 2015/07/29 20:51:50, pthatcher1 wrote: > > > > On 2015/07/29 05:28:13, juberti1 wrote: > > > > > The port becomes connecting after the socket is connected... that seems > > > > > confusing. > > > > > > > > It's already like that in the existing code, where connected_ = false even > > > after > > > > the socket is connected. It's because port connected != socket > connected. > > > > It's a consequence of using the word "connected" for both the port and the > > > > socket, even though they have different meanings. The only way around it > > is > > > to > > > > think of a different name than "connected" for a port that is ready to > send. > > > > > > "ready"? > > > If we change the name, we'd better change all three. > > > For now I changed the comments to make it less confusing. Please let me know > > if > > > we should change the enum names. > > > > how about STATE_CONNECTING (init), STATE_CONNECTED (ready to send allocate), > > STATE_READY (received allocate success), STATE_DISCONNECTED (TCP connection > > died) > > That sounds fine. The only strange part is that connected() will be implemented > with "state_ == STATE_READY", which is confusing. Well, I can change the method to ready() :) STATE_CONNECTING and STATE_DISCONNECTED is essentially the same state in which nothing should be sent out. Guess we just want the logical difference?
On 2015/07/30 21:35:05, honghaiz3 wrote: > https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc > File webrtc/p2p/base/turnport.cc (right): > > https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport.cc:341: // TCP port becomes CONNECTING after the socket > is connected, > On 2015/07/30 21:29:16, pthatcher1 wrote: > > On 2015/07/30 20:47:40, juberti1 wrote: > > > On 2015/07/29 21:44:13, honghaiz3 wrote: > > > > On 2015/07/29 20:51:50, pthatcher1 wrote: > > > > > On 2015/07/29 05:28:13, juberti1 wrote: > > > > > > The port becomes connecting after the socket is connected... that > seems > > > > > > confusing. > > > > > > > > > > It's already like that in the existing code, where connected_ = false > even > > > > after > > > > > the socket is connected. It's because port connected != socket > > connected. > > > > > It's a consequence of using the word "connected" for both the port and > the > > > > > socket, even though they have different meanings. The only way around > it > > > is > > > > to > > > > > think of a different name than "connected" for a port that is ready to > > send. > > > > > > > > "ready"? > > > > If we change the name, we'd better change all three. > > > > For now I changed the comments to make it less confusing. Please let me > know > > > if > > > > we should change the enum names. > > > > > > how about STATE_CONNECTING (init), STATE_CONNECTED (ready to send allocate), > > > STATE_READY (received allocate success), STATE_DISCONNECTED (TCP connection > > > died) > > > > That sounds fine. The only strange part is that connected() will be > implemented > > with "state_ == STATE_READY", which is confusing. > Well, I can change the method to ready() :) > > STATE_CONNECTING and STATE_DISCONNECTED is essentially the same state in which > nothing should be sent out. Guess we just want the logical difference? yes; since right now DISCONNECTED is a terminal state, I think the distinction is important. If we support reconnect then we may remove the distinction.
Patchset #7 (id:300001) has been deleted
Patchset #7 (id:320001) has been deleted
Patchset #7 (id:340001) has been deleted
Using Clear() + Check state when creating permission requests. PTAL. https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:643: if (port_state_ == DISCONNECTED) { On 2015/07/30 20:47:40, juberti1 wrote: > On 2015/07/30 18:38:27, honghaiz3 wrote: > > On 2015/07/30 18:22:33, juberti1 wrote: > > > On 2015/07/29 22:00:10, pthatcher1 wrote: > > > > On 2015/07/29 21:44:13, honghaiz3 wrote: > > > > > On 2015/07/29 20:51:50, pthatcher1 wrote: > > > > > > On 2015/07/29 05:28:13, juberti1 wrote: > > > > > > > Could we just flush pending packets when the connection drops, to > > avoid > > > > this > > > > > > > problem? > > > > > > > > > > > > We could add a StunRequestManager::RemoveAll that would then call > > > > > Thread::Clear. > > > > > > And we could call request_manager_->RemoveAll() here. That sounds > like > > a > > > > > good > > > > > > approach. > > > > > Actually there is a request_manager_->Clear method there and I tried > that, > > > but > > > > > that does not eliminate all requests. Some the requests are the > permission > > > > > requests which are generated when TurnEntry is created, which is created > > > from > > > > > Connection, which is created when > > > > > a new candidate is received in the signaling path... > > > > > > > > But if RequestManager called thread_->Clear(), I think it would remove all > > of > > > > the messages. > > > > > > I think Clear() + a check during permission creation would be the best > > approach. > > That means we will keep all the changes on the PortState and check the state > at > > the time of creating permission instead of sending the packet here, right? > > right. We would ASSERT if we get a packet when STATE_DISCONNECTED. Done. https://codereview.webrtc.org/1247573002/diff/240001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/1247573002/diff/240001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:34: enum PortState { On 2015/07/30 20:47:40, juberti1 wrote: > This should be TurnPortState, since it is at global scope I have moved this inside class TurnPort.
Overall looks good. One comment below. https://codereview.webrtc.org/1247573002/diff/360001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/360001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1292: if (port_->connected()) { Might be better to return NULL from CreateConnection, so that we don't silently eat permission requests.
https://codereview.webrtc.org/1247573002/diff/360001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1247573002/diff/360001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1292: if (port_->connected()) { On 2015/07/31 06:04:42, juberti1 wrote: > Might be better to return NULL from CreateConnection, so that we don't silently > eat permission requests. Done. This is a different version I prepared yesterday (did not send out).
lgtm
The CQ bit was checked by honghaiz@webrtc.org
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/1247573002/#ps380001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247573002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247573002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/6628)
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247573002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247573002/380001
Message was sent while issue was closed.
Committed patchset #8 (id:380001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b19eba3d4bbc70ece91d524e21e2e9d4253ff7a9 Cr-Commit-Position: refs/heads/master@{#9671} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
