Revert "Revert 235213 "android: forwader2: Simplify Forwarder implementa...""

Revert "Revert 235213 "android: forwader2: Simplify Forwarder implementa...""

This relands digit@'s CL that completely reworks the traffic forwarding logic
and that should guarantee in particular that select() never blocks without a
good reason (i.e. that we are not leaking Forwarder instances due to the fact
that their internal thread would be blocked on select()).

The initial problem with r235213 was only the use of PipeNotifier which added
two file descriptors for each Forwarder instance (in addition to the two
sockets the Forwarder instance operates on). We were exceeding the file
descriptor limit (1024) on the intl_ko_th_vi Telemetry page set which is not
surprising since Chrome easily keeps 256 sockets around in its socket pool.
Therefore this change simply removes the PipeNotifier since we were not (yet)
using it anyway.

The initial problem was reproduced and the fix was tested as well with:
tools/perf/run_measurement -v --browser=android-chromium-testshell \
    --show-stdout page_cycler tools/perf/page_sets/intl_ko_th_vi.json

Note that a next step would be to have all the Forwarder instances operate on a
common thread owned by DeviceListener so that we don't end up with 256 threads.

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

[digit1, 2014-01-14 10:44:59]

lgtm

No wonder we couldn't find the 'bug' easily. It's a bit sad to let the notifier
go, but at least we have a better forwarder for now.

[Philippe, 2014-01-14 10:51:44]

Thanks David! I will add the notifier back (but a shared one this time)
once I'm back on this :): https://codereview.chromium.org/60033002/


On Tue, Jan 14, 2014 at 11:45 AM, <digit@chromium.org> wrote:

> lgtm
>
> No wonder we couldn't find the 'bug' easily. It's a bit sad to let the
> notifier
> go, but at least we have a better forwarder for now.
>
>
>
> https://codereview.chromium.org/137923004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Philippe, 2014-01-14 10:52:24]

JFYI Marcus, I'm waiting for you now :)


On Tue, Jan 14, 2014 at 11:51 AM, Philippe Liard <pliard@chromium.org>wrote:

> Thanks David! I will add the notifier back (but a shared one this time)
> once I'm back on this :): https://codereview.chromium.org/60033002/
>
>
> On Tue, Jan 14, 2014 at 11:45 AM, <digit@chromium.org> wrote:
>
>> lgtm
>>
>> No wonder we couldn't find the 'bug' easily. It's a bit sad to let the
>> notifier
>> go, but at least we have a better forwarder for now.
>>
>>
>>
>> https://codereview.chromium.org/137923004/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bulach, 2014-01-14 14:15:01]

lgtm, thanks! small suggestions, feel free to ignore.

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

https://codereview.chromium.org/137923004/diff/110001/tools/android/forwarder...
tools/android/forwarder2/forwarder.cc:47: // State transitions are:
nice documentation, thanks!

if I understood this correctly, looks like T02, T06 and T11 are "exceptions",
right?
should we add some log in ForceClose() to print which transition has occurred
and potentially help with debugging?

https://codereview.chromium.org/137923004/diff/110001/tools/android/forwarder...
tools/android/forwarder2/forwarder.cc:80: void SetPeer(BufferedCopier* peer) {
peer_ = peer; }
nit: CHECK(!peer);

[digit1, 2014-01-14 14:33:00]

They're not really exceptions.

  T02 happens when the forwarder is waiting for data and the device program
closes the connection.
  T06 happens when the forwarder tries to send buffered data to a host program
that has forcibly closed the connection.
  T11 is the same thing, except that the device-side socket is already closed.

All these things will happen during testing, I don't think we need to log them.

[Philippe, 2014-01-14 14:50:57]

Thanks guys!

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

https://codereview.chromium.org/137923004/diff/110001/tools/android/forwarder...
tools/android/forwarder2/forwarder.cc:80: void SetPeer(BufferedCopier* peer) {
peer_ = peer; }
On 2014/01/14 14:15:01, bulach wrote:
> nit: CHECK(!peer);

Yeah, good idea. I made it a DCHECK though if you don't mind :)

[commit-bot: I haz the power, 2014-01-14 14:52:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/137923004/240001

[commit-bot: I haz the power, 2014-01-14 23:50:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/137923004/240001

[commit-bot: I haz the power, 2014-01-15 04:39:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/137923004/240001

[commit-bot: I haz the power, 2014-01-16 05:37:38]

Change committed as 245144