Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1015733002:
Close data pipes when TCP network connection goes down. (Closed)
|
Created:
5 years, 9 months ago by Cutch Modified:
5 years, 9 months ago Reviewers:
jamesr CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClose data pipes when TCP network connection goes down.
- Close data pipes when TCP network connection goes down.
Committed: https://crrev.com/cd34266e673171ffd426ad7768798287fa12bd92
Cr-Commit-Position: refs/heads/master@{#321456}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Messages
Total messages: 18 (4 generated)
johnmccutchan@google.com changed reviewers: + jamesr@google.com
https://codereview.chromium.org/1015733002/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1015733002/diff/1/AUTHORS#newcode240 AUTHORS:240: John McCutchan <john@johnmccutchan.com> To stop presubmit nag.
jamesr@chromium.org changed reviewers: + jamesr@chromium.org - jamesr@google.com
What is your git author configured to? I have mine configured to my @chromium.org account, so commits show up like so: https://chromium.googlesource.com/external/mojo/+/5336e6f4e228f28b210a3a6b132... but are still associated with my github account. https://codereview.chromium.org/1015733002/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1015733002/diff/1/AUTHORS#newcode240 AUTHORS:240: John McCutchan <john@johnmccutchan.com> On 2015/03/17 14:55:41, Cutch wrote: > To stop presubmit nag. why a personal address instead of your @google or @chromium (if you have one) ?
I don't fully understand the need for the direction bit - how will that be used?
On 2015/03/18 18:23:11, jamesr wrote: > What is your git author configured to? I have mine configured to my > @chromium.org account, so commits show up like so: > https://chromium.googlesource.com/external/mojo/+/5336e6f4e228f28b210a3a6b132... > but are still associated with my github account. > > https://codereview.chromium.org/1015733002/diff/1/AUTHORS > File AUTHORS (right): > > https://codereview.chromium.org/1015733002/diff/1/AUTHORS#newcode240 > AUTHORS:240: John McCutchan <mailto:john@johnmccutchan.com> > On 2015/03/17 14:55:41, Cutch wrote: > > To stop presubmit nag. > > why a personal address instead of your @google or @chromium (if you have one) ? Yes, I have my @google address associated with my GitHub account but my git-author is configured to my personal. I can change my Git author on my workstation if there is some issue with me committing with my personal email address.
On 2015/03/18 23:09:20, Cutch wrote: > On 2015/03/18 18:23:11, jamesr wrote: > > What is your git author configured to? I have mine configured to my > > @chromium.org account, so commits show up like so: > > > https://chromium.googlesource.com/external/mojo/+/5336e6f4e228f28b210a3a6b132... > > but are still associated with my github account. > > > > https://codereview.chromium.org/1015733002/diff/1/AUTHORS > > File AUTHORS (right): > > > > https://codereview.chromium.org/1015733002/diff/1/AUTHORS#newcode240 > > AUTHORS:240: John McCutchan <mailto:john@johnmccutchan.com> > > On 2015/03/17 14:55:41, Cutch wrote: > > > To stop presubmit nag. > > > > why a personal address instead of your @google or @chromium (if you have one) > ? > > Yes, I have my @google address associated with my GitHub account but my > git-author is configured to my personal. I can change my Git author on my > workstation if there is some issue with me committing with my personal email > address. AUTHORS exists to keep track of who has signed a CLA. As a Googler, you (probably) aren't authorized to sign a CLA as an individual so you can't go in the AUTHORS file. The easiest way to keep track of the fact that it's OK to have contributions from you is to use your @google or @chromium account.
On 2015/03/18 20:46:59, jamesr wrote: > I don't fully understand the need for the direction bit - how will that be used? With this CL we "notify" the socket owner by closing one or both data pipes. But, that doesn't provide any context (network error, mojo error) associated with the close. In the future I would like to add something to the TCPConnectedSocket interface which reports the context and which pipe (direction) was closed.
On 2015/03/18 20:46:59, jamesr wrote: > I don't fully understand the need for the direction bit - how will that be used? With this CL we "notify" the socket owner by closing one or both data pipes. data pipes, but, that doesn't provide any context (network error, mojo error) associated with the close. In the future I would like to add something to the TCPConnectedSocket interface which reports the context and which pipe was closed.
Let's not add new data speculatively - when we have a way to expose this information in the interface we can plumb the data through. Having this stuff pipe through and go nowhere just makes the code harder to understand and (slightly) less efficient.
PTAL
lgtm https://codereview.chromium.org/1015733002/diff/20001/mojo/services/network/t... File mojo/services/network/tcp_connected_socket_impl.cc (right): https://codereview.chromium.org/1015733002/diff/20001/mojo/services/network/t... mojo/services/network/tcp_connected_socket_impl.cc:122: pending_receive_ = NULL; nullptr. only use NULL in code that has to compile as C https://codereview.chromium.org/1015733002/diff/20001/mojo/services/network/t... mojo/services/network/tcp_connected_socket_impl.cc:202: pending_send_ = NULL; nullptr
The CQ bit was checked by johnmccutchan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jamesr@chromium.org Link to the patchset: https://codereview.chromium.org/1015733002/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015733002/40001
https://codereview.chromium.org/1015733002/diff/20001/mojo/services/network/t... File mojo/services/network/tcp_connected_socket_impl.cc (right): https://codereview.chromium.org/1015733002/diff/20001/mojo/services/network/t... mojo/services/network/tcp_connected_socket_impl.cc:122: pending_receive_ = NULL; On 2015/03/19 21:54:23, jamesr wrote: > nullptr. only use NULL in code that has to compile as C Fixed here and elsewhere. https://codereview.chromium.org/1015733002/diff/20001/mojo/services/network/t... mojo/services/network/tcp_connected_socket_impl.cc:122: pending_receive_ = NULL; On 2015/03/19 21:54:23, jamesr wrote: > nullptr. only use NULL in code that has to compile as C Done.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cd34266e673171ffd426ad7768798287fa12bd92 Cr-Commit-Position: refs/heads/master@{#321456} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
