Remove Thread wrapper class in forwarder2.

Remove Thread wrapper class in forwarder2.

This class unnecessarily reinvented the wheel in a non-RAII way that proved to
be harmful (see r210830).
Also, this class wasn't playing nicely with Chromium's base idioms. For
instance it was impossible to do task posting with it. This implied some
non-idiomatic threading code that was hard to read and reason about (e.g. using
locks, pthread_condition_t...) thus hard to maintain and debug. See
device_listener.cc for instance.

This CL also refactors the shutdown code to ensure that in general all the 
objects are always destroyed on the thread they were created on. This wasn't
the case before.

BUG=242846
R=bulach@chromium.org, digit@google.com

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214338

[Philippe, 2013-07-18 14:04:54]

Adding Marcus on this. This is a big cleanup CL which I realize might be hard to
read. So let me know if you want me to split it :)

[digit1, 2013-07-18 20:08:38]

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/device_controller.cc (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.cc:44: void
DeviceController::AcceptClientSoon() {
nit: I think AcceptHostCommandSoon() / AcceptHostCommand() would be more
descriptive names here

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.cc:106:
listener->SetAdbDataSocket(socket.Pass());
Since you changed the interaction between DeviceController and DeviceListener,
shouldn't you check that the listener is effectively waiting for a new socket
here? Otherwise things could get hairy if the host forwarder sends this command
by mistake.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.cc:108: case command::UNMAP_PORT:
nit:  This should really be called UNLISTEN :-) Let's keep it for another CL
though.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/device_controller.h (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.h:24: 
Can I ask you to add a comment describing the purpose of this class, as in:

// There is a single DeviceController per device_forwarder process,
// and it is in charge of managing all active redirections on the
// device side (one DeviceListener each).

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/device_listener.cc (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_listener.cc:40: scoped_ptr<PipeNotifier>
pipe_notifier(new PipeNotifier());
I assume you can get rid of this and just pass a "new PipeNotifier()" as the
first argument to the DeviceListener constructor below.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/device_listener.h (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_listener.h:20: 
Can I ask you to add a comment that explains what this class does. The code is
becoming more and more complex, and I think that will help review / understand
it better :-)

Here's an example:

// A DeviceListener instance is used in the device_forwarder program to bind
// to a specific device-side |port| and wait for client connections. When a
// connection happens, it informs its creator, through |adb_control_socket|
// and waits for a call to its SetAdbDataSocket() method. When this happens,
// a new internal Forwarder instance is started.

+ explain lifecycle management.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_listener.h:25: // from a map before deleting
it).
This is a bit ambiguous. Can you clarify what "client" means here. Typically it
refers to  a program connecting to the DeviceListener's bound socket :)

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/forwarder.cc (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/forwarder.cc:86: 
I'd recommend adding a comment explaining what the class does, for example:

// Internal class that wraps a helper thread to forward traffic between
|socket1| and |socket2|
// After creating a new instance, call its Start() method to launch operations.
//
// Thread stops automatically if one of the socket disconnects, but ensures that
all buffered
// writes to the other, still live, socket, are written first. When this
happens, the instance will
// delete itself automatically.
//
// The instance will always be destroyed on the same thread that created it.

Question, is possible to force-stop a thread by deleting it or not?

[Philippe, 2013-07-22 15:16:14]

Thanks David!

In addition to addressing your comments, I also had to improve the global
shutdown code (when the daemons are killed). I noticed that the threading
guarantees were not always met :) Sorry again for the big diff.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/device_controller.cc (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.cc:44: void
DeviceController::AcceptClientSoon() {
On 2013/07/18 20:08:39, digit1 wrote:
> nit: I think AcceptHostCommandSoon() / AcceptHostCommand() would be more
> descriptive names here

Indeed.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.cc:106:
listener->SetAdbDataSocket(socket.Pass());
On 2013/07/18 20:08:39, digit1 wrote:
> Since you changed the interaction between DeviceController and DeviceListener,
> shouldn't you check that the listener is effectively waiting for a new socket
> here? Otherwise things could get hairy if the host forwarder sends this
command
> by mistake.

I believe it should not be too risky as it is currently. The DeviceListener
instances are created and destroyed on the same thread this function runs on. If
|listener| is non-null then the listener should be accepting/have accepted a
connection. In case it fails while doing that (on its own internal thread), then
it self-deletes on the controller thread (see DeleteListener() below) which is
also the thread this function is running on. In the worst case the
OnAdbDataSocketReceivedOnInternalThread() callback is queued and executed after
a socket error. This would cause an error when this callback sends a command to
the host but I don't see more risks like use after free (not sure if this is the
kind of things you meant by "hairy" :)). Sorry, not sure this is very clear :)

What do you think?

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.cc:108: case command::UNMAP_PORT:
On 2013/07/18 20:08:39, digit1 wrote:
> nit:  This should really be called UNLISTEN :-) Let's keep it for another CL
> though.

Yeah indeed :) I made the change in this CL since it's a simple change.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/device_controller.h (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.h:24: 
On 2013/07/18 20:08:39, digit1 wrote:
> Can I ask you to add a comment describing the purpose of this class, as in:
> 
> // There is a single DeviceController per device_forwarder process,
> // and it is in charge of managing all active redirections on the
> // device side (one DeviceListener each).

Sure, thanks :)

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.h:30: bool Init(const std::string&
adb_unix_socket);
FYI, I also removed this method in favor of a static factory method (doing
Init() + construction) as I had done before with DeviceListener. Classes should
be designed to be easy to use and hard to use incorrectly :)

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_controller.h:42: typedef IDMap<DeviceListener,
IDMapOwnPointer> ListenersMap;
FYI, I also switched from base::IDMap<> to std::hash_map<>.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/device_listener.cc (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_listener.cc:40: scoped_ptr<PipeNotifier>
pipe_notifier(new PipeNotifier());
On 2013/07/18 20:08:39, digit1 wrote:
> I assume you can get rid of this and just pass a "new PipeNotifier()" as the
> first argument to the DeviceListener constructor below.

Done.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/device_listener.h (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_listener.h:20: 
On 2013/07/18 20:08:39, digit1 wrote:
> Can I ask you to add a comment that explains what this class does. The code is
> becoming more and more complex, and I think that will help review / understand
> it better :-)
> 
> Here's an example:
> 
> // A DeviceListener instance is used in the device_forwarder program to bind
> // to a specific device-side |port| and wait for client connections. When a
> // connection happens, it informs its creator, through |adb_control_socket|
> // and waits for a call to its SetAdbDataSocket() method. When this happens,
> // a new internal Forwarder instance is started.
> 
> + explain lifecycle management.

Thanks.

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/device_listener.h:25: // from a map before deleting
it).
On 2013/07/18 20:08:39, digit1 wrote:
> This is a bit ambiguous. Can you clarify what "client" means here. Typically
it
> refers to  a program connecting to the DeviceListener's bound socket :)

Yeah :)

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/forwarder.cc (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/forwarder.cc:86: 
On 2013/07/18 20:08:39, digit1 wrote:
> I'd recommend adding a comment explaining what the class does, for example:
> 
> // Internal class that wraps a helper thread to forward traffic between
> |socket1| and |socket2|
> // After creating a new instance, call its Start() method to launch
operations.
> //
> // Thread stops automatically if one of the socket disconnects, but ensures
that
> all buffered
> // writes to the other, still live, socket, are written first. When this
> happens, the instance will
> // delete itself automatically.
> //
> // The instance will always be destroyed on the same thread that created it.
> 
> Question, is possible to force-stop a thread by deleting it or not?

Thanks :) I'm not completely sure what you meant with your last question.
Thread::~Thread() joins by default. So Forwarder::~Forwarder() waits for the
thread completion. In our case the situation is a bit specific since the
self-deletion is initiated by the internal thread. Note that the deletion
doesn't happen on the internal thread though since base::Thread must be deleted
on the thread it was created on  (which sounds like a good idea :). So in our
case (although base::Thread::~Thread also guarantees that), it is guaranteed
that the thread completes before the Forwarder instance gets killed. Not sure
this is what you were asking?

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
File tools/android/forwarder2/host_controller.h (right):

https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
tools/android/forwarder2/host_controller.h:29: HostController(int device_port,
FYI, I added a static factory method like I also did with DeviceListener to make
this interface harder to use incorrectly.

https://chromiumcodereview.appspot.com/19478003/diff/63001/tools/android/forw...
File tools/android/forwarder2/device_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/19478003/diff/63001/tools/android/forw...
tools/android/forwarder2/device_forwarder_main.cc:58:
controller_thread_->message_loop_proxy()->PostTask(
FYI, I also added this in the last patch set.

[Philippe, 2013-07-29 13:56:34]

On 2013/07/22 15:16:14, Philippe wrote:
> Thanks David!
> 
> In addition to addressing your comments, I also had to improve the global
> shutdown code (when the daemons are killed). I noticed that the threading
> guarantees were not always met :) Sorry again for the big diff.
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> File tools/android/forwarder2/device_controller.cc (right):
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/device_controller.cc:44: void
> DeviceController::AcceptClientSoon() {
> On 2013/07/18 20:08:39, digit1 wrote:
> > nit: I think AcceptHostCommandSoon() / AcceptHostCommand() would be more
> > descriptive names here
> 
> Indeed.
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/device_controller.cc:106:
> listener->SetAdbDataSocket(socket.Pass());
> On 2013/07/18 20:08:39, digit1 wrote:
> > Since you changed the interaction between DeviceController and
DeviceListener,
> > shouldn't you check that the listener is effectively waiting for a new
socket
> > here? Otherwise things could get hairy if the host forwarder sends this
> command
> > by mistake.
> 
> I believe it should not be too risky as it is currently. The DeviceListener
> instances are created and destroyed on the same thread this function runs on.
If
> |listener| is non-null then the listener should be accepting/have accepted a
> connection. In case it fails while doing that (on its own internal thread),
then
> it self-deletes on the controller thread (see DeleteListener() below) which is
> also the thread this function is running on. In the worst case the
> OnAdbDataSocketReceivedOnInternalThread() callback is queued and executed
after
> a socket error. This would cause an error when this callback sends a command
to
> the host but I don't see more risks like use after free (not sure if this is
the
> kind of things you meant by "hairy" :)). Sorry, not sure this is very clear :)
> 
> What do you think?
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/device_controller.cc:108: case command::UNMAP_PORT:
> On 2013/07/18 20:08:39, digit1 wrote:
> > nit:  This should really be called UNLISTEN :-) Let's keep it for another CL
> > though.
> 
> Yeah indeed :) I made the change in this CL since it's a simple change.
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> File tools/android/forwarder2/device_controller.h (right):
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/device_controller.h:24: 
> On 2013/07/18 20:08:39, digit1 wrote:
> > Can I ask you to add a comment describing the purpose of this class, as in:
> > 
> > // There is a single DeviceController per device_forwarder process,
> > // and it is in charge of managing all active redirections on the
> > // device side (one DeviceListener each).
> 
> Sure, thanks :)
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/device_controller.h:30: bool Init(const std::string&
> adb_unix_socket);
> FYI, I also removed this method in favor of a static factory method (doing
> Init() + construction) as I had done before with DeviceListener. Classes
should
> be designed to be easy to use and hard to use incorrectly :)
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/device_controller.h:42: typedef IDMap<DeviceListener,
> IDMapOwnPointer> ListenersMap;
> FYI, I also switched from base::IDMap<> to std::hash_map<>.
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> File tools/android/forwarder2/device_listener.cc (right):
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/device_listener.cc:40: scoped_ptr<PipeNotifier>
> pipe_notifier(new PipeNotifier());
> On 2013/07/18 20:08:39, digit1 wrote:
> > I assume you can get rid of this and just pass a "new PipeNotifier()" as the
> > first argument to the DeviceListener constructor below.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> File tools/android/forwarder2/device_listener.h (right):
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/device_listener.h:20: 
> On 2013/07/18 20:08:39, digit1 wrote:
> > Can I ask you to add a comment that explains what this class does. The code
is
> > becoming more and more complex, and I think that will help review /
understand
> > it better :-)
> > 
> > Here's an example:
> > 
> > // A DeviceListener instance is used in the device_forwarder program to bind
> > // to a specific device-side |port| and wait for client connections. When a
> > // connection happens, it informs its creator, through |adb_control_socket|
> > // and waits for a call to its SetAdbDataSocket() method. When this happens,
> > // a new internal Forwarder instance is started.
> > 
> > + explain lifecycle management.
> 
> Thanks.
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/device_listener.h:25: // from a map before deleting
> it).
> On 2013/07/18 20:08:39, digit1 wrote:
> > This is a bit ambiguous. Can you clarify what "client" means here. Typically
> it
> > refers to  a program connecting to the DeviceListener's bound socket :)
> 
> Yeah :)
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> File tools/android/forwarder2/forwarder.cc (right):
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/forwarder.cc:86: 
> On 2013/07/18 20:08:39, digit1 wrote:
> > I'd recommend adding a comment explaining what the class does, for example:
> > 
> > // Internal class that wraps a helper thread to forward traffic between
> > |socket1| and |socket2|
> > // After creating a new instance, call its Start() method to launch
> operations.
> > //
> > // Thread stops automatically if one of the socket disconnects, but ensures
> that
> > all buffered
> > // writes to the other, still live, socket, are written first. When this
> > happens, the instance will
> > // delete itself automatically.
> > //
> > // The instance will always be destroyed on the same thread that created it.
> > 
> > Question, is possible to force-stop a thread by deleting it or not?
> 
> Thanks :) I'm not completely sure what you meant with your last question.
> Thread::~Thread() joins by default. So Forwarder::~Forwarder() waits for the
> thread completion. In our case the situation is a bit specific since the
> self-deletion is initiated by the internal thread. Note that the deletion
> doesn't happen on the internal thread though since base::Thread must be
deleted
> on the thread it was created on  (which sounds like a good idea :). So in our
> case (although base::Thread::~Thread also guarantees that), it is guaranteed
> that the thread completes before the Forwarder instance gets killed. Not sure
> this is what you were asking?
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> File tools/android/forwarder2/host_controller.h (right):
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/49001/tools/android/forw...
> tools/android/forwarder2/host_controller.h:29: HostController(int device_port,
> FYI, I added a static factory method like I also did with DeviceListener to
make
> this interface harder to use incorrectly.
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/63001/tools/android/forw...
> File tools/android/forwarder2/device_forwarder_main.cc (right):
> 
>
https://chromiumcodereview.appspot.com/19478003/diff/63001/tools/android/forw...
> tools/android/forwarder2/device_forwarder_main.cc:58:
> controller_thread_->message_loop_proxy()->PostTask(
> FYI, I also added this in the last patch set.

Ping :)

[digit, 2013-07-29 18:11:48]

lgtm. Thanks you for all the additional comments :)

[digit, 2013-07-29 18:11:59]

https://chromiumcodereview.appspot.com/19478003/diff/14002/tools/android/forw...
File tools/android/forwarder2/device_controller.cc (right):

https://chromiumcodereview.appspot.com/19478003/diff/14002/tools/android/forw...
tools/android/forwarder2/device_controller.cc:73: base::Unretained(this)));
I'm a bit sad that we have to create a heap-allocated callback just to ensure
that one of the methods of this instance is always called on scope exit. It
looks like a helper scoped class could do this with much less code. I'm
surprised there isn't anything under base/ that can do that (after all, there is
base/scoped_clear_enno.h which seems even more specialized).

Please don't create such a helper class, your code is good, this is not
performance sensitive, it was  just a general observation :-)

https://chromiumcodereview.appspot.com/19478003/diff/14002/tools/android/forw...
tools/android/forwarder2/device_controller.cc:83: DeviceListener* const listener
= listener_it == listeners_.end() ?
nit: I think the style guides recommends breaking lines before non-assignment
operators. I'd also suggest adding parentheses to isolate the conditions from
the results, as in:

  DeviceListener* const listener = (listener_it == listeners_end.())
      ? static_cast<DeviceListener*>(NULL) : listener_it->second.get();

Note: if this was passed through "git cl format" though, forget what I say here
:)

[Philippe, 2013-07-30 09:37:18]

Thanks David!

https://chromiumcodereview.appspot.com/19478003/diff/14002/tools/android/forw...
File tools/android/forwarder2/device_controller.cc (right):

https://chromiumcodereview.appspot.com/19478003/diff/14002/tools/android/forw...
tools/android/forwarder2/device_controller.cc:73: base::Unretained(this)));
On 2013/07/29 18:11:59, digit wrote:
> I'm a bit sad that we have to create a heap-allocated callback just to ensure
> that one of the methods of this instance is always called on scope exit. It
> looks like a helper scoped class could do this with much less code. I'm
> surprised there isn't anything under base/ that can do that (after all, there
is
> base/scoped_clear_enno.h which seems even more specialized).
> 
> Please don't create such a helper class, your code is good, this is not
> performance sensitive, it was  just a general observation :-)

Yeah I agree this is sad :/

Some of the code in base/ could be a bit more generic so that it could better
integrate with the STL. For instance base::Callback<> could provide an
operator() (in addition to Run()) to match the functional objects of the STL
(that don't do heap allocations under the hood).

Then ScopedClosureRunner could be:
template <typename F>
class ScopedClosureRunner {
 public:
  explicit ScopedClosureRunner(const F& function) : function_(function) {}
    
  ~ScopedClosureRunner() { function(); }

 private:
  Function function_;
};

Then this code could be replaced with the less-flexible but "zero cost":
base::ScopedClosureRunner accept_next_client(
    std::bind1st(
        std::memfun(&DeviceController::AcceptHostCommandSoon), this));
              
Readability is another story though :)

https://chromiumcodereview.appspot.com/19478003/diff/14002/tools/android/forw...
tools/android/forwarder2/device_controller.cc:83: DeviceListener* const listener
= listener_it == listeners_.end() ?
On 2013/07/29 18:11:59, digit wrote:
> nit: I think the style guides recommends breaking lines before non-assignment
> operators. I'd also suggest adding parentheses to isolate the conditions from
> the results, as in:
> 
>   DeviceListener* const listener = (listener_it == listeners_end.())
>       ? static_cast<DeviceListener*>(NULL) : listener_it->second.get();
> 
> Note: if this was passed through "git cl format" though, forget what I say
here
> :)

Done :)

[bulach, 2013-07-30 11:11:36]

lgtm, thanks!! just some questions below:

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
File tools/android/forwarder2/forwarder.cc (right):

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
tools/android/forwarder2/forwarder.cc:132: if (HANDLE_EINTR(select(nfds,
&read_fds, &write_fds, NULL, NULL)) <= 0) {
there are too many layers, so apologies for what could be a silly question :)
I think we always start roughly in this order:
WPR
forwarder
chrome

however, if chrome for some reason hangs, or say, didn't finish the test, the
next bot run will do the above, and afaict, it'd kill each one of the above
individually...

once we kill wpr, or the host forwarder, would this be propagated all the way
down here?

or in another way: is there a case where both read and write fds here are valid
and we'd not receive an EINTR, so that this thread would never be able to join?

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:86: // each redirection. Note
that redirections can happen with many devices
nit: is "redirection" the same as "forwarding" here?

[pliard, 2013-07-30 11:50:20]

Thanks Marcus!

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
File tools/android/forwarder2/forwarder.cc (right):

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
tools/android/forwarder2/forwarder.cc:132: if (HANDLE_EINTR(select(nfds,
&read_fds, &write_fds, NULL, NULL)) <= 0) {
On 2013/07/30 11:11:37, bulach wrote:
> there are too many layers, so apologies for what could be a silly question :)
> I think we always start roughly in this order:
> WPR
> forwarder
> chrome
> 
> however, if chrome for some reason hangs, or say, didn't finish the test, the
> next bot run will do the above, and afaict, it'd kill each one of the above
> individually...
> 
> once we kill wpr, or the host forwarder, would this be propagated all the way
> down here?
> 
> or in another way: is there a case where both read and write fds here are
valid
> and we'd not receive an EINTR, so that this thread would never be able to
join?

This is a non-trivial question so not silly at all :)

I'm not sure I fully understand it but I will do my best: select() should return
as soon as there is any event on at least one of the sockets.
So any side doing a close() would trigger the self-deletion of this class.Note
that this class runs both on the host and the device.

The global shutdown situation (through a kill) here could be improved by using a
"PipeNotifier" as we do in other places. Currently if one of the two processes
(host_forwarder or device_forwarder) is killed, we are (rather unintentionally)
relying on the fact that the file descriptors are automatically closed. The
other side would see a close on the corresponding socket. I will send a follow
up CL to improve that (by using a PipeNotifier here). I don't expect it to cause
any behavioral change though.

What do you think?

Are you referring to the bug where you could see ~16 threads blocked here on
select()?
I will try to reproduce the hang with this CL.

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:86: // each redirection. Note
that redirections can happen with many devices
On 2013/07/30 11:11:37, bulach wrote:
> nit: is "redirection" the same as "forwarding" here?

Yeah :) Done.

[pliard, 2013-07-30 12:03:05]

FYI, I'm landing this now so that I can monitor the tree for a few hours
(should be fine though since it passed the 10 x86 emulator instances test
:)). But this doesn't mean that I'm closing the discussion :)


On Tue, Jul 30, 2013 at 1:50 PM, <pliard@google.com> wrote:

> Thanks Marcus!
>
>
>
> https://codereview.chromium.**org/19478003/diff/89001/tools/**
>
android/forwarder2/forwarder.**cc<https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/forwarder.cc>
> File tools/android/forwarder2/**forwarder.cc (right):
>
> https://codereview.chromium.**org/19478003/diff/89001/tools/**
>
android/forwarder2/forwarder.**cc#newcode132<https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/forwarder.cc#newcode132>
> tools/android/forwarder2/**forwarder.cc:132: if (HANDLE_EINTR(select(nfds,
> &read_fds, &write_fds, NULL, NULL)) <= 0) {
> On 2013/07/30 11:11:37, bulach wrote:
>
>> there are too many layers, so apologies for what could be a silly
>>
> question :)
>
>> I think we always start roughly in this order:
>> WPR
>> forwarder
>> chrome
>>
>
>  however, if chrome for some reason hangs, or say, didn't finish the
>>
> test, the
>
>> next bot run will do the above, and afaict, it'd kill each one of the
>>
> above
>
>> individually...
>>
>
>  once we kill wpr, or the host forwarder, would this be propagated all
>>
> the way
>
>> down here?
>>
>
>  or in another way: is there a case where both read and write fds here
>>
> are valid
>
>> and we'd not receive an EINTR, so that this thread would never be able
>>
> to join?
>
> This is a non-trivial question so not silly at all :)
>
> I'm not sure I fully understand it but I will do my best: select()
> should return as soon as there is any event on at least one of the
> sockets.
> So any side doing a close() would trigger the self-deletion of this
> class.Note that this class runs both on the host and the device.
>
> The global shutdown situation (through a kill) here could be improved by
> using a "PipeNotifier" as we do in other places. Currently if one of the
> two processes (host_forwarder or device_forwarder) is killed, we are
> (rather unintentionally) relying on the fact that the file descriptors
> are automatically closed. The other side would see a close on the
> corresponding socket. I will send a follow up CL to improve that (by
> using a PipeNotifier here). I don't expect it to cause any behavioral
> change though.
>
> What do you think?
>
> Are you referring to the bug where you could see ~16 threads blocked
> here on select()?
> I will try to reproduce the hang with this CL.
>
>
> https://codereview.chromium.**org/19478003/diff/89001/tools/**
>
android/forwarder2/host_**forwarder_main.cc<https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/host_forwarder_main.cc>
> File tools/android/forwarder2/host_**forwarder_main.cc (right):
>
> https://codereview.chromium.**org/19478003/diff/89001/tools/**
>
android/forwarder2/host_**forwarder_main.cc#newcode86<https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/host_forwarder_main.cc#newcode86>
> tools/android/forwarder2/host_**forwarder_main.cc:86: // each redirection.
> Note that redirections can happen with many devices
> On 2013/07/30 11:11:37, bulach wrote:
>
>> nit: is "redirection" the same as "forwarding" here?
>>
>
> Yeah :) Done.
>
>
https://codereview.chromium.**org/19478003/<https://codereview.chromium.org/1...
>

[Philippe, 2013-07-30 12:13:36]

Committed patchset #9 manually as r214338 (presubmit successful).

[bulach, 2013-07-30 13:11:44]

still lgtm :)

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
File tools/android/forwarder2/forwarder.cc (right):

https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/...
tools/android/forwarder2/forwarder.cc:132: if (HANDLE_EINTR(select(nfds,
&read_fds, &write_fds, NULL, NULL)) <= 0) {
On 2013/07/30 11:50:20, pliard wrote:
> On 2013/07/30 11:11:37, bulach wrote:
> > there are too many layers, so apologies for what could be a silly question
:)
> > I think we always start roughly in this order:
> > WPR
> > forwarder
> > chrome
> > 
> > however, if chrome for some reason hangs, or say, didn't finish the test,
the
> > next bot run will do the above, and afaict, it'd kill each one of the above
> > individually...
> > 
> > once we kill wpr, or the host forwarder, would this be propagated all the
way
> > down here?
> > 
> > or in another way: is there a case where both read and write fds here are
> valid
> > and we'd not receive an EINTR, so that this thread would never be able to
> join?
> 
> This is a non-trivial question so not silly at all :)
> 
> I'm not sure I fully understand it but I will do my best: select() should
return
> as soon as there is any event on at least one of the sockets.
> So any side doing a close() would trigger the self-deletion of this class.Note
> that this class runs both on the host and the device.
> 
> The global shutdown situation (through a kill) here could be improved by using
a
> "PipeNotifier" as we do in other places. Currently if one of the two processes
> (host_forwarder or device_forwarder) is killed, we are (rather
unintentionally)
> relying on the fact that the file descriptors are automatically closed. The
> other side would see a close on the corresponding socket. I will send a follow
> up CL to improve that (by using a PipeNotifier here). I don't expect it to
cause
> any behavioral change though.
> 
> What do you think?
> 
> Are you referring to the bug where you could see ~16 threads blocked here on
> select()?
> I will try to reproduce the hang with this CL.

thanks for the detailed explanation!
agree with going ahead with this patch, just some more details on the case I was
thinking:
1) run a test where chrome loads a URL, but for whatever reason, it doesn't
complete (or perhaps that request had a Keep-Alive)

2) way, way higher up would eventually fail the test, like a timeout on
buildbot, or telemetry, etc... at this stage, all components (WPR, forwarder,
chrome), are left behind in an unknown state... afaict, both forwarders would be
blocked at this select() here.

3) next run comes along. at this point, I think we'd first try to kill the WPR,
then forwarder, then chrome... so if I understand correctly, the flow at this
point is:
3.1) kill WPR
3.2) close the socket.
3.3) unblock here, close the host forwarder
3.4) which would unblock the device forwarder
4) start the forwarder
5) start chrome, run the test

So I guess it will fit together nicely, right? :)

...unless.. (3.4) and (4) are async, and potentially 4 happens before... I guess
with the other change making the forwarder API synchronized at the python level
this shouldn't happen, but anyways..

[pliard, 2013-07-30 13:42:15]

Thanks Marcus! Ah I understand better now :) I had this hanging issue in
mind and misunderstood.

So you are exactly right and the forwarder commands (including connection
redirection setup/teardown and process killing) are all synchronous now.
The process that is used to trigger the commands (and that talks to the
daemon through a Unix Domain Socket) only returns after the daemon notified
it (which happens when the operations completed). We already tear
down/setup race conditions in the past because we were using plain kill
commands which are asynchronous. This was replaced will a '--kill-server'
parameter to make it synchronous.

And yeah I intend to re-land the other CL soon once Pawel approved the CL
in testserver.py. I had yet another fun debugging session with that :)


On Tue, Jul 30, 2013 at 3:11 PM, <bulach@chromium.org> wrote:

> still lgtm :)
>
>
>
> https://codereview.chromium.**org/19478003/diff/89001/tools/**
>
android/forwarder2/forwarder.**cc<https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/forwarder.cc>
> File tools/android/forwarder2/**forwarder.cc (right):
>
> https://codereview.chromium.**org/19478003/diff/89001/tools/**
>
android/forwarder2/forwarder.**cc#newcode132<https://codereview.chromium.org/19478003/diff/89001/tools/android/forwarder2/forwarder.cc#newcode132>
> tools/android/forwarder2/**forwarder.cc:132: if (HANDLE_EINTR(select(nfds,
> &read_fds, &write_fds, NULL, NULL)) <= 0) {
> On 2013/07/30 11:50:20, pliard wrote:
>
>> On 2013/07/30 11:11:37, bulach wrote:
>> > there are too many layers, so apologies for what could be a silly
>>
> question :)
>
>> > I think we always start roughly in this order:
>> > WPR
>> > forwarder
>> > chrome
>> >
>> > however, if chrome for some reason hangs, or say, didn't finish the
>>
> test, the
>
>> > next bot run will do the above, and afaict, it'd kill each one of
>>
> the above
>
>> > individually...
>> >
>> > once we kill wpr, or the host forwarder, would this be propagated
>>
> all the way
>
>> > down here?
>> >
>> > or in another way: is there a case where both read and write fds
>>
> here are
>
>> valid
>> > and we'd not receive an EINTR, so that this thread would never be
>>
> able to
>
>> join?
>>
>
>  This is a non-trivial question so not silly at all :)
>>
>
>  I'm not sure I fully understand it but I will do my best: select()
>>
> should return
>
>> as soon as there is any event on at least one of the sockets.
>> So any side doing a close() would trigger the self-deletion of this
>>
> class.Note
>
>> that this class runs both on the host and the device.
>>
>
>  The global shutdown situation (through a kill) here could be improved
>>
> by using a
>
>> "PipeNotifier" as we do in other places. Currently if one of the two
>>
> processes
>
>> (host_forwarder or device_forwarder) is killed, we are (rather
>>
> unintentionally)
>
>> relying on the fact that the file descriptors are automatically
>>
> closed. The
>
>> other side would see a close on the corresponding socket. I will send
>>
> a follow
>
>> up CL to improve that (by using a PipeNotifier here). I don't expect
>>
> it to cause
>
>> any behavioral change though.
>>
>
>  What do you think?
>>
>
>  Are you referring to the bug where you could see ~16 threads blocked
>>
> here on
>
>> select()?
>> I will try to reproduce the hang with this CL.
>>
>
> thanks for the detailed explanation!
> agree with going ahead with this patch, just some more details on the
> case I was thinking:
> 1) run a test where chrome loads a URL, but for whatever reason, it
> doesn't complete (or perhaps that request had a Keep-Alive)
>
> 2) way, way higher up would eventually fail the test, like a timeout on
> buildbot, or telemetry, etc... at this stage, all components (WPR,
> forwarder, chrome), are left behind in an unknown state... afaict, both
> forwarders would be blocked at this select() here.
>
> 3) next run comes along. at this point, I think we'd first try to kill
> the WPR, then forwarder, then chrome... so if I understand correctly,
> the flow at this point is:
> 3.1) kill WPR
> 3.2) close the socket.
> 3.3) unblock here, close the host forwarder
> 3.4) which would unblock the device forwarder
> 4) start the forwarder
> 5) start chrome, run the test
>
> So I guess it will fit together nicely, right? :)
>
> ...unless.. (3.4) and (4) are async, and potentially 4 happens before...
> I guess with the other change making the forwarder API synchronized at
> the python level this shouldn't happen, but anyways..
>
>
https://codereview.chromium.**org/19478003/<https://codereview.chromium.org/1...
>