Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
348803003:
Refactor tcp socket and unix domain socket. (Closed)
|
Created:
6 years, 6 months ago by byungchul Modified:
6 years, 5 months ago Reviewers:
Ryan Sleevi, Randy Smith (Not in Mondays), gunsch, keybuk, darin (slow to review), Jana, mmenke CC:
cbentzel+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRefactor tcp socket.
This is a pre-requisite for http server refactoring,
https://codereview.chromium.org/296053012/.
1) Introduce SocketLibevent for tcp and unix domain sockets.
2) TCPSocketLibevent utilizes SocketLibevent.
3) For backward compatibility, TCPSocketLibevent::AdoptConnectedSocket()
allows the empty address.
BUG=371906
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282121
Patch Set 1 #Patch Set 2 : Fix compilation errors #Patch Set 3 : Fix unittests and compilation errors #Patch Set 4 : Fix net_unittests for iOS #
Total comments: 52
Patch Set 5 : Addressing comments #
Total comments: 8
Patch Set 6 : Rebased #Patch Set 7 : Addressed comments and fixed unittests for chrome os. #
Total comments: 30
Patch Set 8 : Rebased #Patch Set 9 : Remove unix domain socket change #Patch Set 10 : Fix commit message #
Total comments: 13
Patch Set 11 : Rebased #Patch Set 12 : Change logic calling TCPSocketLibevent::RecordFastOpenStatus() #Patch Set 13 : Call TCPSocketLibevent::RecordFastOpenStatus() always except on ERR_IO_PENDING #Patch Set 14 : Revert the behavior change of calling RecordFastOpenStatus() #
Total comments: 2
Messages
Total messages: 48 (0 generated)
Please review this, a pre-requite for https://codereview.chromium.org/296053012/.
On 2014/06/20 08:23:35, byungchul wrote: > Please review this, a pre-requite for > https://codereview.chromium.org/296053012/. May not get back to you today, but I'm (finally) starting to review this. Thank you for your patience.
Some comments. I intent to get you more later today, and hopefully get you some on the other CL tomorrow. I like your cleanups to the underlying socket code! https://codereview.chromium.org/348803003/diff/60001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/server_socket... net/socket/server_socket.h:30: virtual int ListenWithAddressAndPort(const std::string& address_string, need to include <string> https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... File net/socket/socket_libevent.cc (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:56: // members manually. I think it would make more sense to give SockaddrStorage a copy constructor and assignment operator. More robust against incorrect usage. Great job noticing this problem, by the way. Suppose switching the struct to use a union would work as well, though you'd have to modify call sites. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:76: DCHECK_EQ(socket_fd_, kInvalidSocket); Put expected before actual (Matters more for EXPECT_EQ and ASSERT_EQ, but think it's simplest to be consistent). Know you're just copying the old order here. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:81: address_family == AF_UNIX ? 0 : IPPROTO_TCP); Maybe DCHECK on address family? Or just take protocol as an argument. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:99: DCHECK_EQ(socket_fd_, kInvalidSocket); nit: Switch order (Not going to point out the rest of these). https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:169: DCHECK(!callback.is_null()); Maybe throw in a DCHECK(!IsConnected()); while you're here? https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:176: } nit: Should be consistent about using braces for single-line if statements. Suggest standardizing on not using them, since that's more common. Believe there are a number of other cases where you're using them. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:420: return rv == OK ? OK : MapConnectError(errno); the first "OK" should be a "0" https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... File net/socket/socket_libevent.h (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:22: // Socket class to provide asynchronous read/write operations on top of posix nit: "on top of the posix" https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:23: // socket api. It support AF_INET, AF_INET6, and AF_UNIX address. Though it nit: "It supports", "addresses" https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:25: // AdoptConnectedSocket() if it is connected already by posix connect() api. I don't think we should mention datagram sockets here. We have other classes for that. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:32: // or AF_UNIX. Otherwise, return an net error. nit: "a net" https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:94: IOBuffer* read_buf_; Why isn't this a scoped_refptr? https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:100: IOBuffer* write_buf_; Why isn't this a scoped_refptr? https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:111: base::ThreadChecker thread_checker_; Why use ThreadChecker instead of inheriting from NonThreadSafe? I don't have a strong preference, the network stack just tends to use the latter. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.h (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.h:24: // to connect to server sockect. nit: "to a server socket" (+to, -c) https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.h:35: // POSIX socket interfaces. This comment is wrong. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... File net/socket/unix_domain_server_socket_posix.cc (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:14: #include "net/socket/unix_domain_client_socket_posix.h" nit: Remove redundant include. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:47: NOTIMPLEMENTED(); nit: Need to include base/logging.h for this. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:51: int UnixDomainServerSocket::ListenWithAddressAndPort( Both of these functions seem less than ideal. My suggestion: Move listen methods out of ServerSocket, and just have a comment there that different implementors have different listen functions. Open to other ideas. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:61: return ERR_ADDRESS_INVALID; nit: Use braces when either part of an if statement takes up more than one line. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:10: #include "base/basictypes.h" nit: include macros.h instead, to make sure we get OVERRIDE and DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:27: typedef base::Callback<bool (uid_t user_id, gid_t group_id)> AuthCallback; Should include callback.h or callback_forward.h. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:36: gid_t* group_id); You should include the headers for uid_t and git_t. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:46: private: nit: Blank line before private. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:56: scoped_ptr<SocketLibevent> accept_socket_; nit: Include base/memory/scoped_ptr.h (Parent interface uses it, but think it's best to make dependencies explicit)
https://codereview.chromium.org/348803003/diff/60001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/server_socket... net/socket/server_socket.h:30: virtual int ListenWithAddressAndPort(const std::string& address_string, On 2014/06/26 15:36:10, mmenke wrote: > need to include <string> Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... File net/socket/socket_libevent.cc (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:56: // members manually. On 2014/06/26 15:36:11, mmenke wrote: > I think it would make more sense to give SockaddrStorage a copy constructor and > assignment operator. More robust against incorrect usage. Great job noticing > this problem, by the way. > > Suppose switching the struct to use a union would work as well, though you'd > have to modify call sites. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:76: DCHECK_EQ(socket_fd_, kInvalidSocket); On 2014/06/26 15:36:11, mmenke wrote: > Put expected before actual (Matters more for EXPECT_EQ and ASSERT_EQ, but think > it's simplest to be consistent). Know you're just copying the old order here. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:81: address_family == AF_UNIX ? 0 : IPPROTO_TCP); On 2014/06/26 15:36:11, mmenke wrote: > Maybe DCHECK on address family? Or just take protocol as an argument. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:99: DCHECK_EQ(socket_fd_, kInvalidSocket); On 2014/06/26 15:36:11, mmenke wrote: > nit: Switch order (Not going to point out the rest of these). Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:169: DCHECK(!callback.is_null()); On 2014/06/26 15:36:11, mmenke wrote: > Maybe throw in a DCHECK(!IsConnected()); while you're here? I have a bit concern about calling IsConnected() which does recv() internally. And, DoConnect() will return net error if it is already connected. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:176: } On 2014/06/26 15:36:10, mmenke wrote: > nit: Should be consistent about using braces for single-line if statements. > Suggest standardizing on not using them, since that's more common. Believe > there are a number of other cases where you're using them. Done. Though I understand it is common, it introduced the heartbleed issue by if (condtion) goto error; goto error; which could be caught easily if there was {}. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:420: return rv == OK ? OK : MapConnectError(errno); On 2014/06/26 15:36:11, mmenke wrote: > the first "OK" should be a "0" Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... File net/socket/socket_libevent.h (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:22: // Socket class to provide asynchronous read/write operations on top of posix On 2014/06/26 15:36:11, mmenke wrote: > nit: "on top of the posix" Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:23: // socket api. It support AF_INET, AF_INET6, and AF_UNIX address. Though it On 2014/06/26 15:36:11, mmenke wrote: > nit: "It supports", "addresses" Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:25: // AdoptConnectedSocket() if it is connected already by posix connect() api. On 2014/06/26 15:36:11, mmenke wrote: > I don't think we should mention datagram sockets here. We have other classes > for that. Removed. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:32: // or AF_UNIX. Otherwise, return an net error. On 2014/06/26 15:36:11, mmenke wrote: > nit: "a net" Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:94: IOBuffer* read_buf_; On 2014/06/26 15:36:11, mmenke wrote: > Why isn't this a scoped_refptr? Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:100: IOBuffer* write_buf_; On 2014/06/26 15:36:11, mmenke wrote: > Why isn't this a scoped_refptr? Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.h:111: base::ThreadChecker thread_checker_; On 2014/06/26 15:36:11, mmenke wrote: > Why use ThreadChecker instead of inheriting from NonThreadSafe? I don't have a > strong preference, the network stack just tends to use the latter. According to thread_checker.h, it is preferable to NonThreadSafe unless derived classes have chance to change thread by DetachFromThread() because it is protected member of NonThreadSafe. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.h (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.h:24: // to connect to server sockect. On 2014/06/26 15:36:11, mmenke wrote: > nit: "to a server socket" (+to, -c) Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.h:35: // POSIX socket interfaces. On 2014/06/26 15:36:11, mmenke wrote: > This comment is wrong. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... File net/socket/unix_domain_server_socket_posix.cc (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:14: #include "net/socket/unix_domain_client_socket_posix.h" On 2014/06/26 15:36:12, mmenke wrote: > nit: Remove redundant include. Not redundant. It's for client socket. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:47: NOTIMPLEMENTED(); On 2014/06/26 15:36:11, mmenke wrote: > nit: Need to include base/logging.h for this. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:51: int UnixDomainServerSocket::ListenWithAddressAndPort( On 2014/06/26 15:36:12, mmenke wrote: > Both of these functions seem less than ideal. My suggestion: Move listen > methods out of ServerSocket, and just have a comment there that different > implementors have different listen functions. Open to other ideas. You suggested ListenWithAddressAndPort() in base server socket might be useful for tcp in the other CL. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:61: return ERR_ADDRESS_INVALID; On 2014/06/26 15:36:11, mmenke wrote: > nit: Use braces when either part of an if statement takes up more than one > line. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:10: #include "base/basictypes.h" On 2014/06/26 15:36:12, mmenke wrote: > nit: include macros.h instead, to make sure we get OVERRIDE and > DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:27: typedef base::Callback<bool (uid_t user_id, gid_t group_id)> AuthCallback; On 2014/06/26 15:36:12, mmenke wrote: > Should include callback.h or callback_forward.h. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:36: gid_t* group_id); On 2014/06/26 15:36:12, mmenke wrote: > You should include the headers for uid_t and git_t. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:46: private: On 2014/06/26 15:36:12, mmenke wrote: > nit: Blank line before private. Done. https://codereview.chromium.org/348803003/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:56: scoped_ptr<SocketLibevent> accept_socket_; On 2014/06/26 15:36:12, mmenke wrote: > nit: Include base/memory/scoped_ptr.h (Parent interface uses it, but think it's > best to make dependencies explicit) Done.
https://codereview.chromium.org/348803003/diff/70001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/348803003/diff/70001/net/base/net_util.cc#new... net/base/net_util.cc:512: addr_len = other.addr_len; You need to initialize addr here. https://codereview.chromium.org/348803003/diff/70001/net/socket/tcp_socket_li... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/348803003/diff/70001/net/socket/tcp_socket_li... net/socket/tcp_socket_libevent.cc:184: return ERR_INVALID_HANDLE; Think ERR_SOCKET_NOT_CONNECTED makes more sense here. Also means behavior isn't changed. https://codereview.chromium.org/348803003/diff/70001/net/socket/tcp_socket_li... net/socket/tcp_socket_libevent.cc:295: return; I don't think this test gets us anything... Not that it really hurts us. https://codereview.chromium.org/348803003/diff/70001/net/socket/tcp_socket_li... net/socket/tcp_socket_libevent.cc:327: const CompletionCallback& callback) { You're currently using the pattern: public: int Blah() {return DoBlah();} private: int DoBlah() { int rv = socket_->Blah(&DoBlahComplete); if (rv == ERR_IO_PENDING) return rv; // Handle result here. } void DoBlahComplete(int rv) { // Handle result here. callback.Run(rv); } Often duplicating much of the code to handle the result. I think there's too much duplicated code this way. One alternative to avoid that duplication: public: int Blah() { int rv = socket_->Blah(&DoBlahComplete); return HandleBlahResult(rv); } private: void DoBlahComplete(int rv) { callback.Run(HandleBlahResult(rv)); } int HandleBlahResult(int rv) { // Handle result here, return new result. }
https://codereview.chromium.org/348803003/diff/70001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/348803003/diff/70001/net/base/net_util.cc#new... net/base/net_util.cc:512: addr_len = other.addr_len; On 2014/06/26 20:04:00, mmenke wrote: > You need to initialize addr here. addr is already initialied to &addr_storage by default ctor, and addr is const. Cannot change it. https://codereview.chromium.org/348803003/diff/70001/net/socket/tcp_socket_li... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/348803003/diff/70001/net/socket/tcp_socket_li... net/socket/tcp_socket_libevent.cc:184: return ERR_INVALID_HANDLE; On 2014/06/26 20:04:00, mmenke wrote: > Think ERR_SOCKET_NOT_CONNECTED makes more sense here. Also means behavior isn't > changed. Done. https://codereview.chromium.org/348803003/diff/70001/net/socket/tcp_socket_li... net/socket/tcp_socket_libevent.cc:295: return; On 2014/06/26 20:04:00, mmenke wrote: > I don't think this test gets us anything... Not that it really hurts us. Done. https://codereview.chromium.org/348803003/diff/70001/net/socket/tcp_socket_li... net/socket/tcp_socket_libevent.cc:327: const CompletionCallback& callback) { On 2014/06/26 20:04:00, mmenke wrote: > You're currently using the pattern: > > public: > int Blah() {return DoBlah();} > > private: > int DoBlah() { > int rv = socket_->Blah(&DoBlahComplete); > > if (rv == ERR_IO_PENDING) > return rv; > > // Handle result here. > } > > void DoBlahComplete(int rv) { > // Handle result here. > > callback.Run(rv); > } > > Often duplicating much of the code to handle the result. I think there's too > much duplicated code this way. One alternative to avoid that duplication: > > public: > int Blah() { > int rv = socket_->Blah(&DoBlahComplete); > return HandleBlahResult(rv); > } > > private: > void DoBlahComplete(int rv) { > callback.Run(HandleBlahResult(rv)); > } > > int HandleBlahResult(int rv) { > // Handle result here, return new result. > } Done.
PTAL
On 2014/06/27 21:38:18, byungchul wrote: > PTAL Will do. Carefully digging through and comparing codepaths and tests, so I'm hoping this is the last time I need to do more than look at responses to my comments.
On 2014/06/30 16:36:12, mmenke wrote: > On 2014/06/27 21:38:18, byungchul wrote: > > PTAL > > Will do. Carefully digging through and comparing codepaths and tests, so I'm > hoping this is the last time I need to do more than look at responses to my > comments. Would you mind separating out the unix_domain code into yet another CL? I'm pretty much done reviewing the rest (Still need to look over the fastopen stuff, but should finish that after lunch), and believe it should separate out pretty trivially (If this requires undo effort, feel free to just say so). No rush on splitting it out to another CL - I'll start reviewing it here. I don't expect it to take me nearly as long, but just from a landing code standpoint, 2000 lines is a lot of code, and it's a lot of code to revert if something breaks.
On 2014/06/30 16:54:20, mmenke wrote: > On 2014/06/30 16:36:12, mmenke wrote: > > On 2014/06/27 21:38:18, byungchul wrote: > > > PTAL > > > > Will do. Carefully digging through and comparing codepaths and tests, so I'm > > hoping this is the last time I need to do more than look at responses to my > > comments. > > Would you mind separating out the unix_domain code into yet another CL? I'm > pretty much done reviewing the rest (Still need to look over the fastopen stuff, > but should finish that after lunch), and believe it should separate out pretty > trivially (If this requires undo effort, feel free to just say so). > > No rush on splitting it out to another CL - I'll start reviewing it here. I > don't expect it to take me nearly as long, but just from a landing code > standpoint, 2000 lines is a lot of code, and it's a lot of code to revert if > something breaks. *undue effort.
On 2014/06/30 16:54:42, mmenke wrote: > On 2014/06/30 16:54:20, mmenke wrote: > > On 2014/06/30 16:36:12, mmenke wrote: > > > On 2014/06/27 21:38:18, byungchul wrote: > > > > PTAL > > > > > > Will do. Carefully digging through and comparing codepaths and tests, so > I'm > > > hoping this is the last time I need to do more than look at responses to my > > > comments. > > > > Would you mind separating out the unix_domain code into yet another CL? I'm > > pretty much done reviewing the rest (Still need to look over the fastopen > stuff, > > but should finish that after lunch), and believe it should separate out pretty > > trivially (If this requires undo effort, feel free to just say so). > > > > No rush on splitting it out to another CL - I'll start reviewing it here. I > > don't expect it to take me nearly as long, but just from a landing code > > standpoint, 2000 lines is a lot of code, and it's a lot of code to revert if > > something breaks. > > *undue effort. Okay, will do.
May not get to the Unix Domain part of the CL today, but if not, will get to it tomorrow. https://codereview.chromium.org/348803003/diff/110001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/348803003/diff/110001/net/base/net_util.cc#ne... net/base/net_util.cc:511: void SockaddrStorage::operator=(const SockaddrStorage& other) { On 2014/06/27 00:08:36, byungchul wrote: > On 2014/06/26 20:04:00, mmenke wrote: > > You need to initialize addr here. > > addr is already initialied to &addr_storage by default ctor, and addr is const. > Cannot change it. Ahh...right. Mind adding a comment along those lines, for people for whom that isn't immediately obvious? https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... File net/socket/socket_libevent.h (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:26: class SocketLibevent : public base::MessageLoopForIO::Watcher{ nit: Space before open brace. https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:32: // or AF_UNIX. Otherwise, returns a net error. We now DCHECK on address family. https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:49: // Multiple outstanding requests are not supported. nit: "...requests of the same type..." https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:54: // Waits for next write event. This os called by TCPsocketLibevent for TCP os -> is https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:56: // waiting for write event successfully. Otherwise, returns net_error. nit: "Otherwise, return a net error code." https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:56: // waiting for write event successfully. Otherwise, returns net_error. nit: Think it's worth explicitly saying this must not be called when using Write. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:415: CreateNetLogIPEndPointCallback(address)); Think we should move this up to HandleAcceptCompleted. It's a little confusing to log it in one place on accept, and another place on failure. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:427: int os_error = errno; nit: Can just inline this below. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:427: int os_error = errno; This imposes a non-obvious constraint on SocketLibevent (And the same is true of accessing errno in the read/write functions as well). This should be documented as part of SocketLibevent's connect function. Longer term, may want to consider a more robust way of passing the information, or only logging the network error. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:493: RecordFastOpenStatus(); We used to call this regardless of failure, success, or getting ERR_IO_PENDING. This is a significant change to when we call this, and I believe it's meant to be called both on success and on failure, not sure about ERR_IO_PENDING. It also overwrites errno. So...changes: 1) Need to call this on error. 2) The ERR_IO_PENDING change is enough of a change that we may want to rename the histogram if affects. Should probably contact the person who added the histogram (rdsmith, maybe?) and ask about this. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:527: int flags = 0x20000000; // Magic flag to enable TCP_FASTOPEN. nit: 2 spaces before comment (Know the old code had this issue as well). https://codereview.chromium.org/348803003/diff/110001/net/socket/unix_domain_... File net/socket/unix_domain_listen_socket_posix.cc (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/unix_domain_... net/socket/unix_domain_listen_socket_posix.cc:15: #include <unistd.h> nit: C++ headers should got before C ones. https://codereview.chromium.org/348803003/diff/110001/net/socket/unix_domain_... File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/unix_domain_... net/socket/unix_domain_server_socket_posix.h:9: #include <string> nit: Blank line between C headers and C++ headers.
https://codereview.chromium.org/348803003/diff/110001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/348803003/diff/110001/net/base/net_util.cc#ne... net/base/net_util.cc:511: void SockaddrStorage::operator=(const SockaddrStorage& other) { On 2014/06/30 18:26:51, mmenke wrote: > On 2014/06/27 00:08:36, byungchul wrote: > > On 2014/06/26 20:04:00, mmenke wrote: > > > You need to initialize addr here. > > > > addr is already initialied to &addr_storage by default ctor, and addr is > const. > > Cannot change it. > > Ahh...right. Mind adding a comment along those lines, for people for whom that > isn't immediately obvious? Done. https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... File net/socket/socket_libevent.h (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:26: class SocketLibevent : public base::MessageLoopForIO::Watcher{ On 2014/06/30 18:26:51, mmenke wrote: > nit: Space before open brace. Done. Do you think NET_EXPORT is necessary here? Not necessary if it will be used only by tcp and by unix domain. https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:32: // or AF_UNIX. Otherwise, returns a net error. On 2014/06/30 18:26:51, mmenke wrote: > We now DCHECK on address family. DCHECK() is only for debug build. Modified the comment. https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:49: // Multiple outstanding requests are not supported. On 2014/06/30 18:26:51, mmenke wrote: > nit: "...requests of the same type..." Done. https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:54: // Waits for next write event. This os called by TCPsocketLibevent for TCP On 2014/06/30 18:26:51, mmenke wrote: > os -> is Done. https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:56: // waiting for write event successfully. Otherwise, returns net_error. On 2014/06/30 18:26:51, mmenke wrote: > nit: "Otherwise, return a net error code." Done. https://codereview.chromium.org/348803003/diff/110001/net/socket/socket_libev... net/socket/socket_libevent.h:56: // waiting for write event successfully. Otherwise, returns net_error. On 2014/06/30 18:26:51, mmenke wrote: > nit: Think it's worth explicitly saying this must not be called when using > Write. Done. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:415: CreateNetLogIPEndPointCallback(address)); On 2014/06/30 18:26:51, mmenke wrote: > Think we should move this up to HandleAcceptCompleted. It's a little confusing > to log it in one place on accept, and another place on failure. Done. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:427: int os_error = errno; On 2014/06/30 18:26:51, mmenke wrote: > nit: Can just inline this below. Done. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:427: int os_error = errno; On 2014/06/30 18:26:51, mmenke wrote: > This imposes a non-obvious constraint on SocketLibevent (And the same is true of > accessing errno in the read/write functions as well). This should be documented > as part of SocketLibevent's connect function. Longer term, may want to consider > a more robust way of passing the information, or only logging the network error. Done. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:493: RecordFastOpenStatus(); On 2014/06/30 18:26:51, mmenke wrote: > We used to call this regardless of failure, success, or getting ERR_IO_PENDING. > This is a significant change to when we call this, and I believe it's meant to > be called both on success and on failure, not sure about ERR_IO_PENDING. It > also overwrites errno. > > So...changes: > 1) Need to call this on error. > 2) The ERR_IO_PENDING change is enough of a change that we may want to rename > the histogram if affects. Should probably contact the person who added the > histogram (rdsmith, maybe?) and ask about this. Not sure if it should be called at error as well. If you see the original code, there are 2 places to call RecordFastOpenStatus(): 1) Read() which calls it only on success 2) DidCompleteRead() which calls BEFORE read. Added rdsmith for review. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:527: int flags = 0x20000000; // Magic flag to enable TCP_FASTOPEN. On 2014/06/30 18:26:51, mmenke wrote: > nit: 2 spaces before comment (Know the old code had this issue as well). Done.
@rdsmith, Could you please review this CL and advise me especially where RecordFastOpenStatus() should go? Should it be called even on error?
I'm happy with this (Though a bit nervous with so much moving stuff around at the bottom of the network stack). Want to take another skim, and I'll probably be ready to sign off, though should wait for feedback from rdsmith before landing.
On 2014/06/30 20:37:43, mmenke wrote: > I'm happy with this (Though a bit nervous with so much moving stuff around at > the bottom of the network stack). Want to take another skim, and I'll probably > be ready to sign off, though should wait for feedback from rdsmith before > landing. Thank you very much for your kind and thorough reviewing. Once it is merged, will prepare next CL only with unix domain change.
On 2014/06/30 at 20:40:04, byungchul wrote: > On 2014/06/30 20:37:43, mmenke wrote: > > I'm happy with this (Though a bit nervous with so much moving stuff around at > > the bottom of the network stack). Want to take another skim, and I'll probably > > be ready to sign off, though should wait for feedback from rdsmith before > > landing. > > Thank you very much for your kind and thorough reviewing. Once it is merged, will prepare next CL only with unix domain change. I'm on vacation this week. If it's high priority, I'm willing to review the histogram change (please ping me back in that case), but I've also cc'd (not added as reviewer; he should do that if he accepts the job) Jana (jri@), who's taken over the fast open work from me.
On 2014/06/30 20:47:24, rdsmith wrote: > On 2014/06/30 at 20:40:04, byungchul wrote: > > On 2014/06/30 20:37:43, mmenke wrote: > > > I'm happy with this (Though a bit nervous with so much moving stuff around > at > > > the bottom of the network stack). Want to take another skim, and I'll > probably > > > be ready to sign off, though should wait for feedback from rdsmith before > > > landing. > > > > Thank you very much for your kind and thorough reviewing. Once it is merged, > will prepare next CL only with unix domain change. > > I'm on vacation this week. If it's high priority, I'm willing to review the > histogram change (please ping me back in that case), but I've also cc'd (not > added as reviewer; he should do that if he accepts the job) Jana (jri@), who's > taken over the fast open work from me. This CL doesn't change the internal of RecordFastOpenStatus(). It changes the place to call it. The original places are 1) in Read() only when it succeeds read(). 2) in DidCompleteRead() at the first line regardless of error. This change calls RecordFastOpenStatus() only when read() succeeds. Wonder if it is okay or not.
Other than the histogram and these comments, this LGTM. https://codereview.chromium.org/348803003/diff/170001/net/socket/socket_libev... File net/socket/socket_libevent.h (right): https://codereview.chromium.org/348803003/diff/170001/net/socket/socket_libev... net/socket/socket_libevent.h:46: // if connect event happens with error. |callback| is called with a net error code, not errno, though errno is set. This goes for the others as well. https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.h (right): https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.h:69: bool IsValid() const { return socket_ != NULL; } This is a behavioral change - if Open failed, it used to return false (Assuming the socket open function returned kInvalidSocket on failure). I think it's safest to replace this with "return socket_ != NULL && socket_->IsValid();" Could make sure socket_ is reset if open fails as well, if you want, but I think we should have both safety checks here, as a precaution. https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.h:69: bool IsValid() const { return socket_ != NULL; } nit: Shouldn't inline functions NamedLikeThis (Know this was wrong before).
Just reviewed the one section Matt requested my opinion on; let me know if you want more from me. I'll ping Jana offline to make sure he agrees with my histogram suggestion. https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:493: RecordFastOpenStatus(); On 2014/06/30 19:38:35, byungchul wrote: > On 2014/06/30 18:26:51, mmenke wrote: > > We used to call this regardless of failure, success, or getting > ERR_IO_PENDING. > > This is a significant change to when we call this, and I believe it's meant to > > be called both on success and on failure, not sure about ERR_IO_PENDING. It > > also overwrites errno. > > > > So...changes: > > 1) Need to call this on error. > > 2) The ERR_IO_PENDING change is enough of a change that we may want to rename > > the histogram if affects. Should probably contact the person who added the > > histogram (rdsmith, maybe?) and ask about this. > > Not sure if it should be called at error as well. If you see the original code, > there are 2 places to call RecordFastOpenStatus(): > 1) Read() which calls it only on success > 2) DidCompleteRead() which calls BEFORE read. > > Added rdsmith for review. Arggh. My read of the code (memory isn't clear) is that this was broken as written; it's called only on success in the synchronous case, and on both success and failure in the async cases. So I think that means you should: * Change the histogram name; suffix an _A or something. * Add an extra argument to the function indicating whether this is success or error, and make sure it's called in both cases (don't call it on IO_PENDING--the goal here is that the first read on the socket will be after the handshake, so it can figure out how the handshake happened). * Keep the behavior in RecordFastOpenStatus() of ignoring the call if we aren't in {FAST,SLOW}_CONNECT_RETURN; if we aren't, that means we've already figured out how the initial handshake went. * Add two more entries in the enum for {FAST,SLOW}_CONNECT_RETURN error, indicating that the fast open connection failed (or, at least, we never managed to use the socket after we did the connection in a way that suggests the problem might have to do with the fast open). But I'd like Jana to evaluate this plan (Matt, your opinion is also welcome). I'll ping him for that eval. https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket.cc File net/socket/tcp_socket.cc (right): https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket.c... net/socket/tcp_socket.cc:9: #include "base/location.h" I presume this addition is actually needed (i.e. that base/location.h should have been here all along and that wasn't showing up in compilation because it's been removed from some other .h file)?
https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket.cc File net/socket/tcp_socket.cc (right): https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket.c... net/socket/tcp_socket.cc:9: #include "base/location.h" On 2014/07/07 19:46:42, rdsmith wrote: > I presume this addition is actually needed (i.e. that base/location.h should > have been here all along and that wasn't showing up in compilation because it's > been removed from some other .h file)? I only realized this recently myself, but message_loop.h includes location.h which is where FROM_HERE is defined, but a lot of the other task runner classes do not.
https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.cc:493: RecordFastOpenStatus(); On 2014/07/07 19:46:41, rdsmith wrote: > On 2014/06/30 19:38:35, byungchul wrote: > > On 2014/06/30 18:26:51, mmenke wrote: > > > We used to call this regardless of failure, success, or getting > > ERR_IO_PENDING. > > > This is a significant change to when we call this, and I believe it's meant > to > > > be called both on success and on failure, not sure about ERR_IO_PENDING. It > > > also overwrites errno. > > > > > > So...changes: > > > 1) Need to call this on error. > > > 2) The ERR_IO_PENDING change is enough of a change that we may want to > rename > > > the histogram if affects. Should probably contact the person who added the > > > histogram (rdsmith, maybe?) and ask about this. > > > > Not sure if it should be called at error as well. If you see the original > code, > > there are 2 places to call RecordFastOpenStatus(): > > 1) Read() which calls it only on success > > 2) DidCompleteRead() which calls BEFORE read. > > > > Added rdsmith for review. > > Arggh. My read of the code (memory isn't clear) is that this was broken as > written; it's called only on success in the synchronous case, and on both > success and failure in the async cases. So I think that means you should: > * Change the histogram name; suffix an _A or something. > * Add an extra argument to the function indicating whether this is success or > error, and make sure it's called in both cases (don't call it on IO_PENDING--the > goal here is that the first read on the socket will be after the handshake, so > it can figure out how the handshake happened). > * Keep the behavior in RecordFastOpenStatus() of ignoring the call if we aren't > in {FAST,SLOW}_CONNECT_RETURN; if we aren't, that means we've already figured > out how the initial handshake went. > * Add two more entries in the enum for {FAST,SLOW}_CONNECT_RETURN error, > indicating that the fast open connection failed (or, at least, we never managed > to use the socket after we did the connection in a way that suggests the problem > might have to do with the fast open). > > But I'd like Jana to evaluate this plan (Matt, your opinion is also welcome). > I'll ping him for that eval. Modified to call RecordFastOpenStatus() as before, and added TODO for you to revise histogram because I am afraid not to do it correctly. https://codereview.chromium.org/348803003/diff/170001/net/socket/socket_libev... File net/socket/socket_libevent.h (right): https://codereview.chromium.org/348803003/diff/170001/net/socket/socket_libev... net/socket/socket_libevent.h:46: // if connect event happens with error. On 2014/07/07 19:20:20, mmenke wrote: > |callback| is called with a net error code, not errno, though errno is set. > This goes for the others as well. Done. https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket.cc File net/socket/tcp_socket.cc (right): https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket.c... net/socket/tcp_socket.cc:9: #include "base/location.h" On 2014/07/07 19:57:55, mmenke wrote: > On 2014/07/07 19:46:42, rdsmith wrote: > > I presume this addition is actually needed (i.e. that base/location.h should > > have been here all along and that wasn't showing up in compilation because > it's > > been removed from some other .h file)? > > I only realized this recently myself, but message_loop.h includes location.h > which is where FROM_HERE is defined, but a lot of the other task runner classes > do not. I added this because of compilation error on my machine. https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket_l... File net/socket/tcp_socket_libevent.h (right): https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.h:69: bool IsValid() const { return socket_ != NULL; } On 2014/07/07 19:20:20, mmenke wrote: > nit: Shouldn't inline functions NamedLikeThis (Know this was wrong before). Done. https://codereview.chromium.org/348803003/diff/170001/net/socket/tcp_socket_l... net/socket/tcp_socket_libevent.h:69: bool IsValid() const { return socket_ != NULL; } On 2014/07/07 19:20:20, mmenke wrote: > This is a behavioral change - if Open failed, it used to return false (Assuming > the socket open function returned kInvalidSocket on failure). > > I think it's safest to replace this with "return socket_ != NULL && > socket_->IsValid();" Could make sure socket_ is reset if open fails as well, if > you want, but I think we should have both safety checks here, as a precaution. Done.
Randy, PTAL
On 2014/07/08 00:51:38, byungchul wrote: > Randy, PTAL Yes, it seems that the histogram is broken -- I like Randy's plan, with a couple of suggested mods: (i) It seems to me that we should *always* record on the first read, as long as we don't get IO_PENDING, otherwise, we won't record TFO connection errors on synchronous reads. (ii) Why does it matter if it's asynchronous or synchronous read? I think having one histogram for both is adequate, as long as we're recording the same thing. Am I missing something? That said, I'm fine with leaving this CL with the same behavior as it was prior to this refactor, with the TODO (feel free to assign the TODO to me.)
https://codereview.chromium.org/348803003/diff/170001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/348803003/diff/170001/net/socket/server_socke... net/socket/server_socket.h:24: // Binds the socket and start listening. Destroy the socket to stop Nit: "start" -> "starts" https://codereview.chromium.org/348803003/diff/170001/net/socket/server_socke... net/socket/server_socket.h:29: // a vaild IPv4 or IPv6 address. Otherwise, it returns ERR_ADDRESS_INVALID. Nit: "vaild" -> "valid"
On 2014/07/08 02:15:29, Jana wrote: > On 2014/07/08 00:51:38, byungchul wrote: > > Randy, PTAL > > Yes, it seems that the histogram is broken -- I like Randy's plan, with a couple > of suggested mods: (i) It seems to me that we should *always* record on the > first read, as long as we don't get IO_PENDING, otherwise, we won't record TFO > connection errors on synchronous reads. (ii) Why does it matter if it's > asynchronous or synchronous read? I think having one histogram for both is > adequate, as long as we're recording the same thing. Am I missing something? > > That said, I'm fine with leaving this CL with the same behavior as it was prior > to this refactor, with the TODO (feel free to assign the TODO to me.) Changed to record always except on ERR_IO_PENDING.
https://codereview.chromium.org/348803003/diff/170001/net/socket/server_socket.h File net/socket/server_socket.h (right): https://codereview.chromium.org/348803003/diff/170001/net/socket/server_socke... net/socket/server_socket.h:24: // Binds the socket and start listening. Destroy the socket to stop On 2014/07/08 02:15:40, Jana wrote: > Nit: "start" -> "starts" Done. https://codereview.chromium.org/348803003/diff/170001/net/socket/server_socke... net/socket/server_socket.h:29: // a vaild IPv4 or IPv6 address. Otherwise, it returns ERR_ADDRESS_INVALID. On 2014/07/08 02:15:40, Jana wrote: > Nit: "vaild" -> "valid" Done.
The TFO changes look good to me. I'll LGTM, module Randy concurring.
The CQ bit was checked by byungchul@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/byungchul@chromium.org/348803003/230001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
@keybuk, please review a simple change in device/bluetooth/bluetooth_socket_net.cc @darin, please review a simple change in extensions/browser/api/socket/tcp_socket.cc
On 2014/07/08 17:00:20, byungchul wrote: > The CQ bit was checked by mailto:byungchul@chromium.org NOT LGTM. Please wait for my review; I'm working on it now.
On 2014/07/08 02:15:29, Jana wrote:
> On 2014/07/08 00:51:38, byungchul wrote:
> > Randy, PTAL
>
> Yes, it seems that the histogram is broken -- I like Randy's plan, with a
couple
> of suggested mods: (i) It seems to me that we should *always* record on the
> first read, as long as we don't get IO_PENDING, otherwise, we won't record TFO
> connection errors on synchronous reads. (ii) Why does it matter if it's
> asynchronous or synchronous read? I think having one histogram for both is
> adequate, as long as we're recording the same thing. Am I missing something?
I think we're in agreement, but that's because I think I suggested what you
suggest above. Specifically:
* (i) What I am suggesting in in my fourth * (should have numbered them) is
equivalent to always recording on the first read. We're only in
{FAST,SLOW}_CONNECT_RETURN if we haven't already done a RecordFastOpenStatus(),
which will always be true on the first read.
* (ii) It doesn't matter if it's a sync or async read (or at least, it
shouldn't--the original code was broken). Reading my proposal carefully I note
that I didn't explicitly state that we should make sure to call it on both
paths, just that it was a mistake that we didn't; was that what you were
referring back to? Or was it my mention of a new histogram? The new histogram
is because the old histogram is broken and needs to be retired, not because of a
difference between sync and async paths.
> That said, I'm fine with leaving this CL with the same behavior as it was
prior
> to this refactor, with the TODO (feel free to assign the TODO to me.)
SG. Reviewing that now.
(byungchul@: I don't intend these comments to hold this CL, but I wanted to
understand them before doing my review, which I did want to hold the CL.)
On 2014/07/08 03:43:12, byungchul wrote: > On 2014/07/08 02:15:29, Jana wrote: > > On 2014/07/08 00:51:38, byungchul wrote: > > > Randy, PTAL > > > > Yes, it seems that the histogram is broken -- I like Randy's plan, with a > couple > > of suggested mods: (i) It seems to me that we should *always* record on the > > first read, as long as we don't get IO_PENDING, otherwise, we won't record TFO > > connection errors on synchronous reads. (ii) Why does it matter if it's > > asynchronous or synchronous read? I think having one histogram for both is > > adequate, as long as we're recording the same thing. Am I missing something? > > > > That said, I'm fine with leaving this CL with the same behavior as it was > prior > > to this refactor, with the TODO (feel free to assign the TODO to me.) > > Changed to record always except on ERR_IO_PENDING. My apologies, but I think this is wrong. If we're not changing the histogram name, we shouldn't change it's behavior. Can you move this back to only being called on async success and async everything? (I'm happy with TODOing the fixed histogram for Jana or I.)
On 2014/07/08 19:03:40, rdsmith wrote: > On 2014/07/08 03:43:12, byungchul wrote: > > On 2014/07/08 02:15:29, Jana wrote: > > > On 2014/07/08 00:51:38, byungchul wrote: > > > > Randy, PTAL > > > > > > Yes, it seems that the histogram is broken -- I like Randy's plan, with a > > couple > > > of suggested mods: (i) It seems to me that we should *always* record on the > > > first read, as long as we don't get IO_PENDING, otherwise, we won't record > TFO > > > connection errors on synchronous reads. (ii) Why does it matter if it's > > > asynchronous or synchronous read? I think having one histogram for both is > > > adequate, as long as we're recording the same thing. Am I missing something? > > > > > > That said, I'm fine with leaving this CL with the same behavior as it was > > prior > > > to this refactor, with the TODO (feel free to assign the TODO to me.) > > > > Changed to record always except on ERR_IO_PENDING. > > My apologies, but I think this is wrong. If we're not changing the histogram > name, we shouldn't change it's behavior. Can you move this back to only being > called on async success and async everything? > > (I'm happy with TODOing the fixed histogram for Jana or I.) Done.
Thank you. LGTM.
https://codereview.chromium.org/348803003/diff/250001/device/bluetooth/blueto... File device/bluetooth/bluetooth_socket_net.cc (right): https://codereview.chromium.org/348803003/diff/250001/device/bluetooth/blueto... device/bluetooth/bluetooth_socket_net.cc:10: #include "base/location.h" what from this header is used by this source file?
https://codereview.chromium.org/348803003/diff/250001/device/bluetooth/blueto... File device/bluetooth/bluetooth_socket_net.cc (right): https://codereview.chromium.org/348803003/diff/250001/device/bluetooth/blueto... device/bluetooth/bluetooth_socket_net.cc:10: #include "base/location.h" On 2014/07/08 22:06:26, keybuk wrote: > what from this header is used by this source file? FROM_HERE
lgm
lgtm
The change to extensions/browser/api/socket/tcp_socket.cc LGTM
The CQ bit was checked by byungchul@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/byungchul@chromium.org/348803003/250001
Message was sent while issue was closed.
Change committed as 282121
Message was sent while issue was closed.
https://codereview.chromium.org/348803003/diff/110001/net/socket/unix_domain_... File net/socket/unix_domain_listen_socket_posix.cc (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/unix_domain_... net/socket/unix_domain_listen_socket_posix.cc:15: #include <unistd.h> On 2014/06/30 18:26:51, mmenke wrote: > nit: C++ headers should got before C ones. Done in https://codereview.chromium.org/376323002/. https://codereview.chromium.org/348803003/diff/110001/net/socket/unix_domain_... File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/348803003/diff/110001/net/socket/unix_domain_... net/socket/unix_domain_server_socket_posix.h:9: #include <string> On 2014/06/30 18:26:51, mmenke wrote: > nit: Blank line between C headers and C++ headers. Done in https://codereview.chromium.org/376323002/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
