Make tcp_client_socket_unittest pass on Linux....

Make tcp_client_socket_unittest pass on Linux.
Requires another changeset that puts libevent in third_party;
I'll upload that next.
This is not the final word; it makes too many syscalls
per read.  But it's a start.  

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=2346

[Elliot Glaysher, 2008-09-16 18:01:59]

Drive by comments by a person who is in no way an expert on sockets or file
descriptor code.

http://codereview.chromium.org/3202/diff/621/406
File base/message_loop.h (right):

http://codereview.chromium.org/3202/diff/621/406#newcode372
Line 372: #endif
I assume this is old dead code.

http://codereview.chromium.org/3202/diff/621/403
File base/message_pump_libevent.h (right):

http://codereview.chromium.org/3202/diff/621/403#newcode38
Line 38: int wakeup_pipe_gazinta_;
"gazinta" and "gazouta" are really weird names. Renaming them would be great. A
comment explaining what they are contractions would also be acceptable.

[Evan Martin, 2008-09-16 18:16:53]

Some comments from a skim of the code:

Style:  80 columns, "// Comments like sentences.", simple_getters_like_this()
const { return ...; }, I saw some tabs in some other code you committed so it
might be good to grep, if any arm of an "if" has curlies than they all should,
pointers next to the type not the variable.

Is 'extern "C"' required for libevent callbacks?  I thought extern was only to
turn off name-mangling, which we don't care about here.

Why do you use "::event_set" but plain "read"?

[Mark Mentovai, 2008-09-16 18:34:17]

A few early comments, sorry they're mostly style nits.  This is interesting, I
want to read tcp_client_socket_libevent more closely.

http://codereview.chromium.org/3202/diff/621/406
File base/message_loop.h (right):

http://codereview.chromium.org/3202/diff/621/406#newcode24
Line 24: #elif defined(OS_LINUX)
These should really be OS_POSIX, since we'll want to use this at least for the
time being on the Mac too.

http://codereview.chromium.org/3202/diff/621/406#newcode282
Line 282: return static_cast<base::MessagePumpLibevent*>(pump_.get());
I'm not in love with this (or the existing pump_win), it's doing an end run
around the type system.

http://codereview.chromium.org/3202/diff/621/402
File base/message_pump_libevent.cc (right):

http://codereview.chromium.org/3202/diff/621/402#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
Just 2008 on new files.

http://codereview.chromium.org/3202/diff/621/402#newcode12
Line 12: extern "C"
This should be static or in an anonymous namespace (and the extern "C" doesn't
really help or hurt here.)

http://codereview.chromium.org/3202/diff/621/402#newcode49
Line 49: DCHECK(0);
spacing; prefer DCHECK(false);.

http://codereview.chromium.org/3202/diff/621/402#newcode60
Line 60: wakeup_event_ = 0;
Not really needed, but if you must, use NULL.

http://codereview.chromium.org/3202/diff/621/403
File base/message_pump_libevent.h (right):

http://codereview.chromium.org/3202/diff/621/403#newcode40
Line 40: public:
One space on this and the private that follows.

http://codereview.chromium.org/3202/diff/621/403#newcode44
Line 44: ::event_base *evbase_;
I didn't see the :: on other libevent names, why here?

http://codereview.chromium.org/3202/diff/621/410
File net/base/tcp_client_socket.h (right):

http://codereview.chromium.org/3202/diff/621/410#newcode8
Line 8: #if defined(OS_WIN)
Needs #include "build/build_config.h" or something first for this to work.

http://codereview.chromium.org/3202/diff/621/410#newcode51
Line 51: // If a result is available immediately, return it; otherwise call back
later with the result.
80, a couple lines later too.

http://codereview.chromium.org/3202/diff/621/410#newcode73
Line 73: #else
We've been trying to be explicit about this stuff.

#if defined(OS_WIN)
...
#elif defined(OS_POSIX)
...
#endif

(or #else #error blah #endif)

http://codereview.chromium.org/3202/diff/621/410#newcode74
Line 74: friend void ::TCPClientSocket_libevent_cb(int socket, short flags, void
*context);
80

http://codereview.chromium.org/3202/diff/621/404
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/3202/diff/621/404#newcode8
Line 8: #include "base/message_loop.h"
sort

http://codereview.chromium.org/3202/diff/621/404#newcode18
Line 18: #define INVALID_SOCKET (unsigned)(-1)
C++-style cast?

This shouldn't be inside the namespace (although the preprocessor doesn't care.)

http://codereview.chromium.org/3202/diff/621/404#newcode37
Line 37: event_ = new struct event;
Why not put event_(new struct event) in the initializer list?

http://codereview.chromium.org/3202/diff/621/404#newcode200
Line 200: /* check to see if connect succeeded */
You use // C++-style elsewhere.

http://codereview.chromium.org/3202/diff/621/404#newcode234
Line 234: case WAITING_READ:
Weird indent inconsistent with your other switches.

[Evan Martin, 2008-09-16 18:38:12]

Hopefully this and everyone else will get you going.  :)

http://codereview.chromium.org/3202/diff/621/403
File base/message_pump_libevent.h (right):

http://codereview.chromium.org/3202/diff/621/403#newcode17
Line 17: class MessagePumpLibevent : public MessagePump {
Can you put a comment describing this class?  One line is fine; consider that
many of your readers don't know what libevent is.

http://codereview.chromium.org/3202/diff/621/403#newcode28
Line 28: struct event_base* getEventbase() { return evbase_; }
I think "struct" isn't necessary here in C++.
get_event_base() const { ... }

http://codereview.chromium.org/3202/diff/621/403#newcode44
Line 44: ::event_base *evbase_;
Would it be too much to ask for "event_base_"?

http://codereview.chromium.org/3202/diff/621/410
File net/base/tcp_client_socket.h (right):

http://codereview.chromium.org/3202/diff/621/410#newcode73
Line 73: #else
Even though the Windows side lacks comments (bad reviewers!), please add
comments for these that aren't self-evident.  In particular a name like "buf_"
is pretty vague.

http://codereview.chromium.org/3202/diff/621/404
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/3202/diff/621/404#newcode5
Line 5: #include "net/base/tcp_client_socket.h"
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/3202/diff/621/404#newcode18
Line 18: #define INVALID_SOCKET (unsigned)(-1)
Why unsigned?  socket_ is signed... (?)

http://codereview.chromium.org/3202/diff/621/404#newcode21
Line 21: static int MapPosixError(int err) {
Can you put a comment before this function?

http://codereview.chromium.org/3202/diff/621/404#newcode42
Line 42: delete event_;
Would a scoped_ptr work?

http://codereview.chromium.org/3202/diff/621/404#newcode129
Line 129: ::event_set(event_, socket_, EV_READ|EV_PERSIST,
I don't know the semantics of libevent and this class.  Do we need to worry
about whether event_ is already set?  Or does the wait_state_ DCHECK mean it's
an error to call  this twice?

[dank, 2008-09-16 21:28:58]

On 2008/09/16 18:38:12, Evan Martin wrote:
> http://codereview.chromium.org/3202/diff/621/403#newcode28
> I think "struct" isn't necessary here in C++.

It's hard to not type it for C structs, especially those with
annoyingly short names.


>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

I may still have this wrong in some files... are we consistent in
following the C, then C++ rule?
 
> http://codereview.chromium.org/3202/diff/621/404#newcode129
> Line 129: ::event_set(event_, socket_, EV_READ|EV_PERSIST,
> I don't know the semantics of libevent and this class.  Do we need to worry
> about whether event_ is already set?  Or does the wait_state_ DCHECK mean it's
> an error to call  this twice?

I've added comments in the .h file making it clear it's not allowed
to call this function again until a result is returned, does
that help?

I think I've addressed all the other comments, and have uploaded.
Please re-check...

[Evan Martin, 2008-09-16 22:04:13]

Sorry for commenting so much on style.  I promise it'll feel natural soon. :(

I think the names for the static libevent callbacks are weird.  I think you
should use one of two patterns for the name:
 - either declare the function as a static member of the class
 - or declare it as file-static and use a short name.

http://codereview.chromium.org/3202/diff/429/649
File base/message_loop.h (right):

http://codereview.chromium.org/3202/diff/429/649#newcode280
Line 280: public:
Is there a reason this one needs to be public while the Windows one doesn't?

http://codereview.chromium.org/3202/diff/429/645
File base/message_pump_libevent.cc (right):

http://codereview.chromium.org/3202/diff/429/645#newcode5
Line 5: #include "base/message_pump_libevent.h"
Newline after here.

http://codereview.chromium.org/3202/diff/429/645#newcode8
Line 8: 
No newline here (consider third_party to just be another directory of "our"
code).

http://codereview.chromium.org/3202/diff/429/645#newcode13
Line 13: // Called if a byte is received on the wakeup pipe
period

http://codereview.chromium.org/3202/diff/429/645#newcode19
Line 19: // remove and discard the wakeup byte
sentence case

http://codereview.chromium.org/3202/diff/429/645#newcode22
Line 22: DCHECK(nread == 1);
Is there anything intelligent we should do in the nread != 1 case?  (DCHECKs
only happen in a debug build.)

http://codereview.chromium.org/3202/diff/429/645#newcode23
Line 23: // tell libevent to break out of inner loop
sentence case

http://codereview.chromium.org/3202/diff/429/645#newcode32
Line 32: // Dang, too much real work in the constructor.
Should this be marked TODO()?

http://codereview.chromium.org/3202/diff/429/645#newcode37
Line 37: wakeup_pipe_in_ = -1;
Perhaps these -1s should be the default values set with the
": wakeup_pipe_in_(-1)" syntax near L28?
Then you can make (with the change suggested below) this be something like
if (err)
  return false;

http://codereview.chromium.org/3202/diff/429/645#newcode39
Line 39: memset(wakeup_event_, 0, sizeof(*wakeup_event_));
Maybe we shouldn't even allocate this in the error case?

http://codereview.chromium.org/3202/diff/429/645#newcode49
Line 49: // Did I mention this was too much real work in the constructor?
Perhaps for now at least make an Init() function that returns a bool, call it
from the constructor, and have the constructor look like:
if (!Init())
  NOTREACHED();

http://codereview.chromium.org/3202/diff/429/645#newcode58
Line 58: delete wakeup_event_;
Should this be a scoped_ptr?

http://codereview.chromium.org/3202/diff/429/645#newcode88
Line 88: if (delay > TimeDelta()) {
Is testing against TimeDelta() trying to test whether it's positive?  Is there a
more logical way to express that?

http://codereview.chromium.org/3202/diff/429/646
File base/message_pump_libevent.h (right):

http://codereview.chromium.org/3202/diff/429/646#newcode11
Line 11: // from libevent
sentence case.

http://codereview.chromium.org/3202/diff/429/646#newcode17
Line 17: // Specialized message pump for background network IO on Posix-based
systems
This comment looks like it got chopped off.

http://codereview.chromium.org/3202/diff/429/646#newcode18
Line 18: // TODO: add support for background file IO somehow
can you put a name (or just "port") on this TODO?

http://codereview.chromium.org/3202/diff/429/646#newcode30
Line 30: struct event_base* get_event_base() const { return event_base_; }
extra "struct"  :)

http://codereview.chromium.org/3202/diff/429/646#newcode40
Line 40: // Write end
This comment looks like it got chopped off.

http://codereview.chromium.org/3202/diff/429/646#newcode44
Line 44: // TODO: use friend MessagePumpLibevent_wakeup instead of public
Can you put a name on this TODO?

http://codereview.chromium.org/3202/diff/429/646#newcode52
Line 52: struct event* wakeup_event_;
Can you put a comment on what this is?

http://codereview.chromium.org/3202/diff/429/654
File net/SConscript (right):

http://codereview.chromium.org/3202/diff/429/654#newcode229
Line 229: 'base/tcp_client_socket_unittest.cc',
Can you alphabetize this?

http://codereview.chromium.org/3202/diff/429/653
File net/base/tcp_client_socket.h (right):

http://codereview.chromium.org/3202/diff/429/653#newcode53
Line 53: // TODO: support multiple outstanding requests?
Can you put names on these TODOs?

http://codereview.chromium.org/3202/diff/429/653#newcode74
Line 74: // TODO: use friend TCPClientSocket_libevent_cb instead of public
Can you put names on these TODOs?

http://codereview.chromium.org/3202/diff/429/653#newcode75
Line 75: void OnLibeventNotification(short flags);
Can you document this function here?

Is "short" really the proper type?  I think we don't use that elsewhere, instead
preferring either "int" or a specific number of bits (int32_t or whatever).

http://codereview.chromium.org/3202/diff/429/647
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/3202/diff/429/647#newcode12
Line 12: #include "net/base/tcp_client_socket.h"
I guess since this is an implementation of the tcp_client_socket.h header, this
#include should be the topmost in the file.

http://codereview.chromium.org/3202/diff/429/647#newcode21
Line 21: static void TCPClientSocket_libevent_cb(int socket, short flags, void*
context) {
80 columns

http://codereview.chromium.org/3202/diff/429/647#newcode31
Line 31: // TODO: fill this with a real conversion table
name on TODO

http://codereview.chromium.org/3202/diff/429/647#newcode57
Line 57: const struct addrinfo* ai = current_ai_;
extra "struct"

http://codereview.chromium.org/3202/diff/429/647#newcode79
Line 79: TCPClientSocket_libevent_cb, this);
can you line this up with the opening paren above?

http://codereview.chromium.org/3202/diff/429/647#newcode82
Line 82: event_.get());
can you line this up with the opening paren above?

http://codereview.chromium.org/3202/diff/429/647#newcode92
Line 92: // No ignorable errors!
Does this mean "no errors are ignorable so we should fail" or that "we haven't
had an ignorable error so we should fail"?

http://codereview.chromium.org/3202/diff/429/647#newcode128
Line 128: DCHECK(callback); // null callback not allowed?
I don't understand this comment.

http://codereview.chromium.org/3202/diff/429/647#newcode192
Line 192: if (-1 == flags)
Do we care if this fcntl fails?

http://codereview.chromium.org/3202/diff/429/647#newcode215
Line 215: // check to see if connect succeeded
sentence case

http://codereview.chromium.org/3202/diff/429/647#newcode218
Line 218: if (getsockopt(socket_, SOL_SOCKET, SO_ERROR, (char*)&error_code,
&len) < 0) {
Can you use a C++-style cast?  I guess it's a reinterpret_cast here.

http://codereview.chromium.org/3202/diff/429/647#newcode222
Line 222: // And await next callback.  Haven't seen this case yet myself.
I don't understand this comment.  Do we want to wait for the next callback, or
should we die with a NOTREACHED()?

http://codereview.chromium.org/3202/diff/429/647#newcode225
Line 225: error_code == EADDRNOTAVAIL ||
can you line this with the opening paren above?

else if (... &&
         (error_code == ... ||
          error_code == ...)) {

http://codereview.chromium.org/3202/diff/429/647#newcode232
Line 232: const struct addrinfo* next = current_ai_->ai_next;
extra "struct"

http://codereview.chromium.org/3202/diff/429/647#newcode237
Line 237: result = MapPosixError(error_code);
two-space tabs here and below

http://codereview.chromium.org/3202/diff/429/647#newcode250
Line 250: int n;
You don't need C-style declarations of variables at the top.  :)

Would you mind using a more descriptive name than "n"?

http://codereview.chromium.org/3202/diff/429/647#newcode264
Line 264: // TODO: can we tell why it closed, and return a more informative
message?
Can you put a name on this TODO?

[Mark Mentovai, 2008-09-16 22:07:37]

http://codereview.chromium.org/3202/diff/429/648
File base/message_loop.cc (right):

http://codereview.chromium.org/3202/diff/429/648#newcode16
Line 16: #if defined(OS_LINUX)
I still see a few OS_LINUXes that should probably be changed to OS_POSIX.

http://codereview.chromium.org/3202/diff/429/653
File net/base/tcp_client_socket.h (right):

http://codereview.chromium.org/3202/diff/429/653#newcode74
Line 74: // TODO: use friend TCPClientSocket_libevent_cb instead of public
Oh, ok, now that I see what's going on here...

TCPClientSocket_libevent_cb should be a _static_ member function, that'll give
it access to private members without needing to give them special access or make
them friendly.

Actually, you can just make OnLibeventNotification be the callback function (if
static) and have it do the callout directly.

http://codereview.chromium.org/3202/diff/429/653#newcode94
Line 94: enum WaitState {
Style: enums before methods within a public/protected/private section.

http://codereview.chromium.org/3202/diff/429/647
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/3202/diff/429/647#newcode15
Line 15: #ifndef INVALID_SOCKET
Are we expecting this to actually be defined by anything else?  It's a
Windowsism, and isn't this non-Windows code?  This would be much better as a
static (or anonymous-namespaced) const int, I think.

http://codereview.chromium.org/3202/diff/429/647#newcode21
Line 21: static void TCPClientSocket_libevent_cb(int socket, short flags, void*
context) {
80

http://codereview.chromium.org/3202/diff/429/647#newcode59
Line 59: 
DCHECK not already WAITING_CONNECT?

http://codereview.chromium.org/3202/diff/429/647#newcode194
Line 194: if (fcntl(socket_, F_SETFL, flags | O_NONBLOCK))
I like to see != 0 on calls like this.

http://codereview.chromium.org/3202/diff/429/647#newcode225
Line 225: error_code == EADDRNOTAVAIL ||
If you're going to catch EADDRNOTAVAIL here, does it make sense to catch
EADDRINUSE too?

http://codereview.chromium.org/3202/diff/429/647#newcode231
Line 231: // This address failed, try next one in list.
Weird inentation here and in the next few else cases.

http://codereview.chromium.org/3202/diff/429/647#newcode259
Line 259: }
default: NOTREACHED();

Won't -Wswitch (enabled at -Wall) warn here otherwise?

http://codereview.chromium.org/3202/diff/429/647#newcode281
Line 281: 
I am blank for no reason

[darin (slow to review), 2008-09-17 16:12:20]

some comments...

http://codereview.chromium.org/3202/diff/429/648
File base/message_loop.cc (right):

http://codereview.chromium.org/3202/diff/429/648#newcode17
Line 17: #include "base/message_pump_libevent.h"
This new MessagePump can probably be used to support TYPE_IO on the Mac, right?

http://codereview.chromium.org/3202/diff/429/645
File base/message_pump_libevent.cc (right):

http://codereview.chromium.org/3202/diff/429/645#newcode49
Line 49: // Did I mention this was too much real work in the constructor?
+1..  this definitely wants to be a helper function that can just return false
when there is an error.

http://codereview.chromium.org/3202/diff/429/645#newcode85
Line 85: event_base_loop(event_base_, EVLOOP_ONCE);
I had trouble finding where it explains this in the documentation for Libevent,
but does this properly block waiting for an event if there are no pending
events?  And, then if there are pending events, I assume that it only services
exactly one pending event?

http://codereview.chromium.org/3202/diff/429/645#newcode88
Line 88: if (delay > TimeDelta()) {
That's the way that Brett recommends.

http://codereview.chromium.org/3202/diff/429/645#newcode107
Line 107: ScheduleWork();
Why is this needed?  Are you concerned about event_base_loop(EVLOOP_ONCE) not
returning if Quit() is reached while event_base_loop(EVLOOP_ONCE) is still on
the stack?  At least some comments are warranted, since it is not obvious what
is going on.

I'd recommend asserting that Quit is reached only when we are inside a call to
Run().

http://codereview.chromium.org/3202/diff/429/646
File base/message_pump_libevent.h (right):

http://codereview.chromium.org/3202/diff/429/646#newcode30
Line 30: struct event_base* get_event_base() const { return event_base_; }
so is the intent that consumers will grab the event_base and use it to add more
sockets to wait on?  are we sure that we don't want to create a higher level API
to abstract away the libevent calls?  what are you planning on using libevent
for?  async dns as well as polling on sockets?

[Evan Martin, 2008-09-17 22:52:02]

This looks pretty good to me.  I have only minor style comments.  I think I
leave it to Darin to give it the seal of approval (unless he's away, in which
case let me know).

You might wanna grep for "Foo *bar", since we put the star next to the type.  I
wouldn't care too much but sometimes I see both styles on the same line.

http://codereview.chromium.org/3202/diff/459/673
File base/message_pump_libevent.cc (right):

http://codereview.chromium.org/3202/diff/459/673#newcode51
Line 51: int err = pipe(fds);
Since you don't use err except to test it, could these all be
if (pipe(fds) != 0)
  return false;

http://codereview.chromium.org/3202/diff/459/681
File net/base/tcp_client_socket.h (right):

http://codereview.chromium.org/3202/diff/459/681#newcode19
Line 19: #include "base/scoped_ptr.h"
alphabetize

http://codereview.chromium.org/3202/diff/459/675
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/3202/diff/459/675#newcode19
Line 19: const int INVALID_SOCKET = -1;
Is this to match Windows?  It's a bit strange to me for a constant to not be
like kInvalidSocket.

http://codereview.chromium.org/3202/diff/459/675#newcode23
Line 23: static int set_non_blocking(int fd)
SetNonBlocking

[Evan Martin, 2008-09-18 00:09:56]

LGTM