Enable forwarder2 for all our tests.

Enable forwarder2 for all our tests.

Also fixes a bunch of small bugs that I found when we tried to run it last Thursday and we had to revert everthing.

Cherrypicked from downstream. Downstream tree is green after this change.

BUG=146502
Change-Id: I438b61b476f22cf59a26ed64a8b42aa8b74fbac5

[Yaron, 2012-10-01 22:28:43]

lgtm

Not sure this is used by anything upstream yet. I take it you'll undisable tests
after this lands?

https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp
File build/all_android.gyp (left):

https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp#...
build/all_android.gyp:59: '../tools/android/forwarder/forwarder.gyp:forwarder',
I take it you'll remove the old forwarder code when this stabilizes?

https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/fo...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/fo...
build/android/pylib/forwarder.py:157: if self._device_process:
Missed this downstream.. as the device is a client connection to the host
server, I believe it's safer to close it first. We had some issues related to
that with the original forwarder

[felipeg_google, 2012-10-01 22:31:17]

On Mon, Oct 1, 2012 at 3:28 PM, <yfriedman@chromium.org> wrote:

> lgtm
>
> Not sure this is used by anything upstream yet. I take it you'll undisable
> tests
> after this lands?
>

Yes,
I will probably undisable the net_unittests
Do you have suggestions of other tests that are currently already
upstreamed but disabled ?


>
>
> https://chromiumcodereview.**appspot.com/11038005/diff/1/**
>
build/all_android.gyp<https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp>
> File build/all_android.gyp (left):
>
> https://chromiumcodereview.**appspot.com/11038005/diff/1/**
>
build/all_android.gyp#**oldcode59<https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp#oldcode59>
> build/all_android.gyp:59:
> '../tools/android/forwarder/**forwarder.gyp:forwarder',
> I take it you'll remove the old forwarder code when this stabilizes?
>
> https://chromiumcodereview.**appspot.com/11038005/diff/1/**
>
build/android/pylib/forwarder.**py<https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/forwarder.py>
> File build/android/pylib/forwarder.**py (right):
>
> https://chromiumcodereview.**appspot.com/11038005/diff/1/**
>
build/android/pylib/forwarder.**py#newcode157<https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/forwarder.py#newcode157>
> build/android/pylib/forwarder.**py:157: if self._device_process:
> Missed this downstream.. as the device is a client connection to the
> host server, I believe it's safer to close it first. We had some issues
> related to that with the original forwarder
>
>
https://chromiumcodereview.**appspot.com/11038005/<https://chromiumcodereview...
>

[Yaron, 2012-10-01 22:33:49]

On 2012/10/01 22:31:17, felipeg wrote:
> On Mon, Oct 1, 2012 at 3:28 PM, <mailto:yfriedman@chromium.org> wrote:
> 
> > lgtm
> >
> > Not sure this is used by anything upstream yet. I take it you'll undisable
> > tests
> > after this lands?
> >
> 
> Yes,
> I will probably undisable the net_unittests
> Do you have suggestions of other tests that are currently already
> upstreamed but disabled ?
> 
> 
> >
> >
> > https://chromiumcodereview.**appspot.com/11038005/diff/1/**
> >
>
build/all_android.gyp<https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp>
> > File build/all_android.gyp (left):
> >
> > https://chromiumcodereview.**appspot.com/11038005/diff/1/**
> >
>
build/all_android.gyp#**oldcode59<https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp#oldcode59>
> > build/all_android.gyp:59:
> > '../tools/android/forwarder/**forwarder.gyp:forwarder',
> > I take it you'll remove the old forwarder code when this stabilizes?
> >
> > https://chromiumcodereview.**appspot.com/11038005/diff/1/**
> >
>
build/android/pylib/forwarder.**py<https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/forwarder.py>
> > File build/android/pylib/forwarder.**py (right):
> >
> > https://chromiumcodereview.**appspot.com/11038005/diff/1/**
> >
>
build/android/pylib/forwarder.**py#newcode157<https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/forwarder.py#newcode157>
> > build/android/pylib/forwarder.**py:157: if self._device_process:
> > Missed this downstream.. as the device is a client connection to the
> > host server, I believe it's safer to close it first. We had some issues
> > related to that with the original forwarder
> >
> >
>
https://chromiumcodereview.**appspot.com/11038005/%3Chttps://chromiumcoderevi...>
> >

I think there a bunch like URLFetcher* that are either in content_unittests or
unit_tests

[felipeg, 2012-10-01 22:35:25]

https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp
File build/all_android.gyp (left):

https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp#...
build/all_android.gyp:59: '../tools/android/forwarder/forwarder.gyp:forwarder',
On 2012/10/01 22:28:43, Yaron wrote:
> I take it you'll remove the old forwarder code when this stabilizes?

Yep.

https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/fo...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/fo...
build/android/pylib/forwarder.py:157: if self._device_process:
On 2012/10/01 22:28:43, Yaron wrote:
> Missed this downstream.. as the device is a client connection to the host
> server, I believe it's safer to close it first. We had some issues related to
> that with the original forwarder

As we chatted in person: The current C++ code is robust in the sense that it
handles the Kill signal and waits for all the data being forwarded to finish
before closing all sockets nicely.
So it should not matter, but the indeed, killing the device_forwarder first
makes more sense anyway.
I will fix it only upstream and we can wait for it to land in the next merge.
What you think ?

[Yaron, 2012-10-01 22:36:26]

On Mon, Oct 1, 2012 at 3:35 PM,  <felipeg@chromium.org> wrote:
>
> https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp
> File build/all_android.gyp (left):
>
>
https://chromiumcodereview.appspot.com/11038005/diff/1/build/all_android.gyp#...
> build/all_android.gyp:59:
> '../tools/android/forwarder/forwarder.gyp:forwarder',
> On 2012/10/01 22:28:43, Yaron wrote:
>>
>> I take it you'll remove the old forwarder code when this stabilizes?
>
>
> Yep.
>
>
>
https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/fo...
> File build/android/pylib/forwarder.py (right):
>
>
https://chromiumcodereview.appspot.com/11038005/diff/1/build/android/pylib/fo...
> build/android/pylib/forwarder.py:157: if self._device_process:
> On 2012/10/01 22:28:43, Yaron wrote:
>>
>> Missed this downstream.. as the device is a client connection to the
>
> host
>>
>> server, I believe it's safer to close it first. We had some issues
>
> related to
>>
>> that with the original forwarder
>
>
> As we chatted in person: The current C++ code is robust in the sense
> that it handles the Kill signal and waits for all the data being
> forwarded to finish before closing all sockets nicely.
> So it should not matter, but the indeed, killing the device_forwarder
> first makes more sense anyway.
> I will fix it only upstream and we can wait for it to land in the next
> merge.
> What you think ?

Yep, that's fine.
>
> https://chromiumcodereview.appspot.com/11038005/

[Philippe, 2012-10-01 22:38:06]

https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
tools/android/forwarder2/host_forwarder_main.cc:97: return -1;
Sorry to catch this only now. main()'s exit code is later converted into an
unsigned byte. This means that this will return 255. Is that intended?

Moreover, 255 is precisely reserved for "out of range" exit codes.

I would have simply returned 1 here.

[felipeg, 2012-10-01 22:42:40]

https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
tools/android/forwarder2/host_forwarder_main.cc:97: return -1;
On 2012/10/01 22:38:06, Philippe wrote:
> Sorry to catch this only now. main()'s exit code is later converted into an
> unsigned byte. This means that this will return 255. Is that intended?
> 
> Moreover, 255 is precisely reserved for "out of range" exit codes.
> 
> I would have simply returned 1 here.
Yep, the return value doesn't matter here. I just want it to return a non-zero
value.
Just to check if the program has had any error or finished nicely.

[Isaac (away), 2012-10-01 22:45:37]

https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
tools/android/forwarder2/host_forwarder_main.cc:97: return -1;
On 2012/10/01 22:42:41, felipeg1 wrote:
> On 2012/10/01 22:38:06, Philippe wrote:
> > Sorry to catch this only now. main()'s exit code is later converted into an
> > unsigned byte. This means that this will return 255. Is that intended?
> > 
> > Moreover, 255 is precisely reserved for "out of range" exit codes.
> > 
> > I would have simply returned 1 here.
> Yep, the return value doesn't matter here. I just want it to return a non-zero
> value.
> Just to check if the program has had any error or finished nicely.

Please change this to a positive value.

[Philippe, 2012-10-01 22:48:38]

On 2012/10/01 22:45:37, Isaac wrote:
>
https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
> File tools/android/forwarder2/host_forwarder_main.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
> tools/android/forwarder2/host_forwarder_main.cc:97: return -1;
> On 2012/10/01 22:42:41, felipeg1 wrote:
> > On 2012/10/01 22:38:06, Philippe wrote:
> > > Sorry to catch this only now. main()'s exit code is later converted into
an
> > > unsigned byte. This means that this will return 255. Is that intended?
> > > 
> > > Moreover, 255 is precisely reserved for "out of range" exit codes.
> > > 
> > > I would have simply returned 1 here.
> > Yep, the return value doesn't matter here. I just want it to return a
non-zero
> > value.
> > Just to check if the program has had any error or finished nicely.
> 
> Please change this to a positive value.

Yes I agree that this should be updated. >128 exit codes are reserved for signal
terminations. 255 suggests a flaw in the process' exit-logic.

[Philippe, 2012-10-01 23:07:30]

On 2012/10/01 22:48:38, Philippe wrote:
> On 2012/10/01 22:45:37, Isaac wrote:
> >
>
https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
> > File tools/android/forwarder2/host_forwarder_main.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarde...
> > tools/android/forwarder2/host_forwarder_main.cc:97: return -1;
> > On 2012/10/01 22:42:41, felipeg1 wrote:
> > > On 2012/10/01 22:38:06, Philippe wrote:
> > > > Sorry to catch this only now. main()'s exit code is later converted into
> an
> > > > unsigned byte. This means that this will return 255. Is that intended?
> > > > 
> > > > Moreover, 255 is precisely reserved for "out of range" exit codes.
> > > > 
> > > > I would have simply returned 1 here.
> > > Yep, the return value doesn't matter here. I just want it to return a
> non-zero
> > > value.
> > > Just to check if the program has had any error or finished nicely.
> > 
> > Please change this to a positive value.
> 
> Yes I agree that this should be updated. >128 exit codes are reserved for
signal
> terminations. 255 suggests a flaw in the process' exit-logic.

Rectification, the conventions I mentioned earlier are used by Bash. That
doesn't make 255 a good exit code though :)

[felipeg_google, 2012-10-01 23:19:04]

The 255 exit code issue is fixed guys
PTAL again
thanks

On Mon, Oct 1, 2012 at 4:07 PM, <pliard@chromium.org> wrote:

> On 2012/10/01 22:48:38, Philippe wrote:
>
>> On 2012/10/01 22:45:37, Isaac wrote:
>> >
>>
>
> https://chromiumcodereview.**appspot.com/11038005/diff/1/**
>
tools/android/forwarder2/host_**forwarder_main.cc<https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarder2/host_forwarder_main.cc>
>
>> > File tools/android/forwarder2/host_**forwarder_main.cc (right):
>> >
>> >
>>
>
> https://chromiumcodereview.**appspot.com/11038005/diff/1/**
>
tools/android/forwarder2/host_**forwarder_main.cc#newcode97<https://chromiumcodereview.appspot.com/11038005/diff/1/tools/android/forwarder2/host_forwarder_main.cc#newcode97>
>
>> > tools/android/forwarder2/host_**forwarder_main.cc:97: return -1;
>> > On 2012/10/01 22:42:41, felipeg1 wrote:
>> > > On 2012/10/01 22:38:06, Philippe wrote:
>> > > > Sorry to catch this only now. main()'s exit code is later converted
>> into
>> an
>> > > > unsigned byte. This means that this will return 255. Is that
>> intended?
>> > > >
>> > > > Moreover, 255 is precisely reserved for "out of range" exit codes.
>> > > >
>> > > > I would have simply returned 1 here.
>> > > Yep, the return value doesn't matter here. I just want it to return a
>> non-zero
>> > > value.
>> > > Just to check if the program has had any error or finished nicely.
>> >
>> > Please change this to a positive value.
>>
>
>  Yes I agree that this should be updated. >128 exit codes are reserved for
>>
> signal
>
>> terminations. 255 suggests a flaw in the process' exit-logic.
>>
>
> Rectification, the conventions I mentioned earlier are used by Bash. That
> doesn't make 255 a good exit code though :)
>
>
https://chromiumcodereview.**appspot.com/11038005/<https://chromiumcodereview...
>

[Philippe, 2012-10-01 23:25:03]

Thanks Felipe.

LGTM.