|
|
DescriptionDo not delete a connection in the turn port with permission error, refresh error, or binding error.
Even if those error happened, the connection may still be able to receive packets for a while.
If we delete the connections, all packets arriving will be dropped.
BUG=webrtc:6007
R=deadbeef@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/3d77deb29c15bfb8f794ef3413837a0ec0f0c131
Cr-Commit-Position: refs/heads/master@{#13262}
Patch Set 1 : . #
Total comments: 17
Patch Set 2 : . #
Total comments: 14
Patch Set 3 : Address comments #
Total comments: 2
Patch Set 4 : Address Taylor's comment and merge #
Total comments: 1
Patch Set 5 : Change comments #Patch Set 6 : Merge with head #
Total comments: 3
Messages
Total messages: 34 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Do not delete a connection in the turn port with permission error or refresh error. BUG= ========== to ========== Do not delete a connection in the turn port with permission error, refresh error, or binding error. Even if those error happened, the connection may still be able to receive packets for a while. If we delete the connections, all packets arriving will be dropped. BUG= ==========
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org
Description was changed from ========== Do not delete a connection in the turn port with permission error, refresh error, or binding error. Even if those error happened, the connection may still be able to receive packets for a while. If we delete the connections, all packets arriving will be dropped. BUG= ========== to ========== Do not delete a connection in the turn port with permission error, refresh error, or binding error. Even if those error happened, the connection may still be able to receive packets for a while. If we delete the connections, all packets arriving will be dropped. BUG=webrtc:6007 ==========
https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2080: EXPECT_EQ(conn2, FindNextPingableConnectionAndPingIt(&ch)); I only moved this test up so that two tests on tiggeredcheck is put together. Below is the actual added test. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1010: // TODO(honghaiz): Investigate why the test fails with FakeClock. Interestingly, this test fails with an assert error. Will investigate later.
https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2108: cricket::P2PTransportChannel ch("do not ping failed connection", 1, &pa); do not => Do not failed connection => failed connections https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:564: << "Received TURN message while the Turn port is disconnected"; Why would we still process packets if we're STATE_DISCONNECTED? https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:757: kv.second->FailAndPrune(); Shouldn't this Destroy() still, since the whole Port is going away? https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1525: port_->FailAndPruneConnection(ext_addr_); Shouldn't we differentiate between different error types? Some types of errors are pretty fatal (bind error/timeout) and some are not (create permission/error). It seems like for the fatal ones, we should DestroyConnection and for the non-fatal ones we should FailAndPruneConnection().
PTAL. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2108: cricket::P2PTransportChannel ch("do not ping failed connection", 1, &pa); On 2016/06/16 22:34:21, pthatcher1 wrote: > do not => Do not > failed connection => failed connections Done. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:564: << "Received TURN message while the Turn port is disconnected"; On 2016/06/16 22:34:21, pthatcher1 wrote: > Why would we still process packets if we're STATE_DISCONNECTED? Turn port becomes disconnected if there is a refresh error (by calling Close()). Just like what we do for the permission error, if it received packets, we should just still process it instead of dropping it. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:757: kv.second->FailAndPrune(); On 2016/06/16 22:34:21, pthatcher1 wrote: > Shouldn't this Destroy() still, since the whole Port is going away? We may enter this in two cases: 1. Socket error, maybe we should Destroy it. but it won't hurt to just FailAndPrune them. 2. Refresh error. In this case we may run into the same issue as the permission error. Do you think we should handle the two cases differently? https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1525: port_->FailAndPruneConnection(ext_addr_); On 2016/06/16 22:34:21, pthatcher1 wrote: > Shouldn't we differentiate between different error types? Some types of errors > are pretty fatal (bind error/timeout) and some are not (create > permission/error). It seems like for the fatal ones, we should > DestroyConnection and for the non-fatal ones we should FailAndPruneConnection(). I guess the question is if there is a binding error, is there still chance for a connection to receive packet? I think it is possible though data indications. If that is the case, I suggest we keep it in case packets received on that address.
https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:564: << "Received TURN message while the Turn port is disconnected"; On 2016/06/16 23:22:53, honghaiz3 wrote: > On 2016/06/16 22:34:21, pthatcher1 wrote: > > Why would we still process packets if we're STATE_DISCONNECTED? > > Turn port becomes disconnected if there is a refresh error (by calling Close()). > Just like what we do for the permission error, if it received packets, we should > just still process it instead of dropping it. See my comment blow about making a refresh error not call Close(). https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:757: kv.second->FailAndPrune(); On 2016/06/16 23:22:53, honghaiz3 wrote: > On 2016/06/16 22:34:21, pthatcher1 wrote: > > Shouldn't this Destroy() still, since the whole Port is going away? > We may enter this in two cases: > 1. Socket error, maybe we should Destroy it. but it won't hurt to just > FailAndPrune them. > 2. Refresh error. In this case we may run into the same issue as the permission > error. > > Do you think we should handle the two cases differently? Yes. A socket close is fatal. A refresh is perhaps not. We should treat fatal things with Destroy() and non-fatal things differently. The comment for STATE_DISCONNECTED indicates it's fatal. I think we should keep it that way. In that case, we should just make a refresh error not Close() the port, at least not immediately. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1525: port_->FailAndPruneConnection(ext_addr_); On 2016/06/16 23:22:53, honghaiz3 wrote: > On 2016/06/16 22:34:21, pthatcher1 wrote: > > Shouldn't we differentiate between different error types? Some types of > errors > > are pretty fatal (bind error/timeout) and some are not (create > > permission/error). It seems like for the fatal ones, we should > > DestroyConnection and for the non-fatal ones we should > FailAndPruneConnection(). > I guess the question is if there is a binding error, is there still chance for a > connection to receive packet? > I think it is possible though data indications. > If that is the case, I suggest we keep it in case packets received on that > address. Good point. These all seem non-fatal. But the socket closing is still fatal. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:324: return true; std::all_of would work well here.
https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:564: << "Received TURN message while the Turn port is disconnected"; Nit: As long as you're here can you fix the capitalization of "Turn" https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:748: void TurnPort::Close() { This method shouldn't be called "Close" if it doesn't result in the port being closed. https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:755: // Delete all existing connections; stop sending data. This comment needs to be updated. https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:907: rtc::ScopedFakeClock clock; Why is the fake clock needed here and in the test below? https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:946: conn1->Send(data.data(), data.length(), options); Are there any EXPECTS that should be added for the data being sent? https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1011: TEST_F(TurnPortTest, TestConnectionFaildAndPredOnCreatePermissionFailure) { "AndPred" should be "AndPruned"? https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1030: rtc::Thread::Current()->ProcessMessages(500); Is the purpose of ProcessMessages(500) to make sure the connection isn't deleted asynchronously? In that case, please try to get this test working with the fake clock.
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
PTAL. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:564: << "Received TURN message while the Turn port is disconnected"; On 2016/06/21 06:58:26, pthatcher1 wrote: > On 2016/06/16 23:22:53, honghaiz3 wrote: > > On 2016/06/16 22:34:21, pthatcher1 wrote: > > > Why would we still process packets if we're STATE_DISCONNECTED? > > > > Turn port becomes disconnected if there is a refresh error (by calling > Close()). > > Just like what we do for the permission error, if it received packets, we > should > > just still process it instead of dropping it. > > See my comment blow about making a refresh error not call Close(). Changed it back. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:757: kv.second->FailAndPrune(); On 2016/06/21 06:58:26, pthatcher1 wrote: > On 2016/06/16 23:22:53, honghaiz3 wrote: > > On 2016/06/16 22:34:21, pthatcher1 wrote: > > > Shouldn't this Destroy() still, since the whole Port is going away? > > We may enter this in two cases: > > 1. Socket error, maybe we should Destroy it. but it won't hurt to just > > FailAndPrune them. > > 2. Refresh error. In this case we may run into the same issue as the > permission > > error. > > > > Do you think we should handle the two cases differently? > > Yes. A socket close is fatal. A refresh is perhaps not. We should treat fatal > things with Destroy() and non-fatal things differently. The comment for > STATE_DISCONNECTED indicates it's fatal. I think we should keep it that way. > > In that case, we should just make a refresh error not Close() the port, at least > not immediately. Done. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1525: port_->FailAndPruneConnection(ext_addr_); On 2016/06/21 06:58:26, pthatcher1 wrote: > On 2016/06/16 23:22:53, honghaiz3 wrote: > > On 2016/06/16 22:34:21, pthatcher1 wrote: > > > Shouldn't we differentiate between different error types? Some types of > > errors > > > are pretty fatal (bind error/timeout) and some are not (create > > > permission/error). It seems like for the fatal ones, we should > > > DestroyConnection and for the non-fatal ones we should > > FailAndPruneConnection(). > > I guess the question is if there is a binding error, is there still chance for > a > > connection to receive packet? > > I think it is possible though data indications. > > If that is the case, I suggest we keep it in case packets received on that > > address. > > Good point. These all seem non-fatal. But the socket closing is still fatal. Acknowledged. https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:564: << "Received TURN message while the Turn port is disconnected"; On 2016/06/21 18:48:08, Taylor Brandstetter wrote: > Nit: As long as you're here can you fix the capitalization of "Turn" Done. https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:748: void TurnPort::Close() { On 2016/06/21 18:48:07, Taylor Brandstetter wrote: > This method shouldn't be called "Close" if it doesn't result in the port being > closed. We are now closing this for socket error. https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:755: // Delete all existing connections; stop sending data. On 2016/06/21 18:48:07, Taylor Brandstetter wrote: > This comment needs to be updated. See the above comments. Changed it back for this method. https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:907: rtc::ScopedFakeClock clock; On 2016/06/21 18:48:08, Taylor Brandstetter wrote: > Why is the fake clock needed here and in the test below? Removed using FakeClock here and below. https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:946: conn1->Send(data.data(), data.length(), options); On 2016/06/21 18:48:08, Taylor Brandstetter wrote: > Are there any EXPECTS that should be added for the data being sent? Added a test that future packets cannot be sent on the connection. Note: the first data on the connection may still be sent via the data indication. https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1011: TEST_F(TurnPortTest, TestConnectionFaildAndPredOnCreatePermissionFailure) { On 2016/06/21 18:48:08, Taylor Brandstetter wrote: > "AndPred" should be "AndPruned"? Done. https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1030: rtc::Thread::Current()->ProcessMessages(500); On 2016/06/21 18:48:08, Taylor Brandstetter wrote: > Is the purpose of ProcessMessages(500) to make sure the connection isn't deleted > asynchronously? In that case, please try to get this test working with the fake > clock. Done. Fixing an issue in virtualsocketserver when virtualsocketserver is created before the fakeclock is set.
lgtm with nit https://codereview.webrtc.org/2068263003/diff/200001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/2068263003/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:208: void handleRefreshError(); Nit: why handleRefreshError and not HandleRefreshError?
https://codereview.webrtc.org/2068263003/diff/200001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/2068263003/diff/200001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:208: void handleRefreshError(); On 2016/06/22 16:49:54, Taylor Brandstetter wrote: > Nit: why handleRefreshError and not HandleRefreshError? Done.
lgtm, with comment nit https://codereview.webrtc.org/2068263003/diff/220001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2068263003/diff/220001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:527: // It will not be used or pinged except that it can still receive packets. I will not be used or send pings, right? "be pinged" means it receives pings, which it still can receive.
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2068263003/#ps220001 (title: "Address Taylor's comment and merge")
The CQ bit was unchecked by honghaiz@webrtc.org
Message was sent while issue was closed.
Description was changed from ========== Do not delete a connection in the turn port with permission error, refresh error, or binding error. Even if those error happened, the connection may still be able to receive packets for a while. If we delete the connections, all packets arriving will be dropped. BUG=webrtc:6007 ========== to ========== Do not delete a connection in the turn port with permission error, refresh error, or binding error. Even if those error happened, the connection may still be able to receive packets for a while. If we delete the connections, all packets arriving will be dropped. BUG=webrtc:6007 R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/3d77deb29c15bfb8f794ef3413837a0ec0f0c131 Cr-Commit-Position: refs/heads/master@{#13262} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3d77deb29c15bfb8f794ef3413837a0ec0f0c131 Cr-Commit-Position: refs/heads/master@{#13262}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:260001) has been created in https://codereview.webrtc.org/2090833002/ by honghaiz@webrtc.org. The reason for reverting is: It broke webrtc builds..
Message was sent while issue was closed.
juberti@google.com changed reviewers: + juberti@google.com
Message was sent while issue was closed.
https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (left): https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1528: port_->DestroyConnection(ext_addr_); This also seems like an error that should be fatal. https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1503: bool found = port_->FailAndPruneConnection(ext_addr_); Shouldn't we destroy the connection if we can't create a permission for it? We shouldn't be able to receive data if the permission fails.
Message was sent while issue was closed.
https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1503: bool found = port_->FailAndPruneConnection(ext_addr_); On 2016/06/23 21:07:29, juberti wrote: > Shouldn't we destroy the connection if we can't create a permission for it? We > shouldn't be able to receive data if the permission fails. Not if the TURN server doesn't check for CreatePermission, right?
Message was sent while issue was closed.
On 2016/06/23 21:29:23, pthatcher1 wrote: > https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc > File webrtc/p2p/base/turnport.cc (right): > > https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport.cc:1503: bool found = > port_->FailAndPruneConnection(ext_addr_); > On 2016/06/23 21:07:29, juberti wrote: > > Shouldn't we destroy the connection if we can't create a permission for it? We > > shouldn't be able to receive data if the permission fails. > > Not if the TURN server doesn't check for CreatePermission, right? I guess my point is that this seems like such a gross failure that trying to survive in this case seems hopeless.
Message was sent while issue was closed.
On 2016/06/23 21:34:50, juberti wrote: > On 2016/06/23 21:29:23, pthatcher1 wrote: > > > https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc > > File webrtc/p2p/base/turnport.cc (right): > > > > > https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport... > > webrtc/p2p/base/turnport.cc:1503: bool found = > > port_->FailAndPruneConnection(ext_addr_); > > On 2016/06/23 21:07:29, juberti wrote: > > > Shouldn't we destroy the connection if we can't create a permission for it? > We > > > shouldn't be able to receive data if the permission fails. > > > > Not if the TURN server doesn't check for CreatePermission, right? > > I guess my point is that this seems like such a gross failure that trying to > survive in this case seems hopeless. or, put a different way, trying to workaround these gross failures may mask the underlying problem.
Message was sent while issue was closed.
On 2016/06/23 21:35:43, juberti wrote: > On 2016/06/23 21:34:50, juberti wrote: > > On 2016/06/23 21:29:23, pthatcher1 wrote: > > > > > > https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc > > > File webrtc/p2p/base/turnport.cc (right): > > > > > > > > > https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport... > > > webrtc/p2p/base/turnport.cc:1503: bool found = > > > port_->FailAndPruneConnection(ext_addr_); > > > On 2016/06/23 21:07:29, juberti wrote: > > > > Shouldn't we destroy the connection if we can't create a permission for > it? > > We > > > > shouldn't be able to receive data if the permission fails. > > > > > > Not if the TURN server doesn't check for CreatePermission, right? > > > > I guess my point is that this seems like such a gross failure that trying to > > survive in this case seems hopeless. > > or, put a different way, trying to workaround these gross failures may mask the > underlying problem. I choose to error on the side of keeping the connection for a while but marking it as failed and pruned. So basically it won't have chance to send ping or data except for potentially receiving data. It won't mask the underlying problem because transport channel should kick off ICE restart or regathering as if the connections are not there. Having said that, if indeed there is absolutely no hope that the server won't send back data in those case, perhaps we should just destroy the connection (as we do before). |