1094164 - [mozdevice] Port forward will fail if the socket is already bound.

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Zac C (:zac)]

While testing this with combinations of b2g open, I noticed that this can fail silently if the port is already bound by desktop b2g.
STR: 
1. Start a desktopb2g instance with marionette port 2828 open
2. Also connect a device with marionette port 2828 open
3. Use mozdevice 'forward' method to port forward tcp:2828 tcp:2828

mozdevice forward returns 1 regardless of the success of the command.

Repeating this STR manually creates a "error: cannot bind to socket" 

http://hg.mozilla.org/mozilla-central/file/62990ec7ad78/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py#l158

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #0)
> While testing this with combinations of b2g open, I noticed that this can
> fail silently if the port is already bound by desktop b2g.
> STR: 
> 1. Start a desktopb2g instance with marionette port 2828 open
> 2. Also connect a device with marionette port 2828 open
> 3. Use mozdevice 'forward' method to port forward tcp:2828 tcp:2828
> 
> mozdevice forward returns 1 regardless of the success of the command.

This is bad information provided by me. The forward method does in fact return 0 if the command is successful. See http://hg.mozilla.org/mozilla-central/file/62990ec7ad78/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py#l158 for the method we're talking about.

So there's two ways we can resolve this. One would be to modify the place we're calling forward to check that it returns 0, which would be here: http://hg.mozilla.org/mozilla-central/file/62990ec7ad78/testing/mozbase/mozrunner/mozrunner/devices/base.py#l188

The other option would be for the forward method itself to throw an exception if it's unsuccessful. This would be my personal preference but would break anything that currently relies on failed forwards being silently ignored.

Will: How do you think we should proceed here?

Comment 2

[Andrew Halberstadt [:ahal]]

+1 to throwing an exception. I don't like using return codes for checking success because it is all too easy for consumers to ignore.

Comment 3

[William Lachance (:wlach)]

Yup, very much in agreement with you and :ahal, throwing an exception is the way to go. If that breaks stuff, we should fix it.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Raise exception if we fail to forward socket connections. v1.0

Comment 5

[William Lachance (:wlach)]

Comment on attachment 8520049 [details] [diff] [review]
Raise exception if we fail to forward socket connections. v1.0

Review of attachment 8520049 [details] [diff] [review]:
-----------------------------------------------------------------

wfm

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10bc5d51b1fc

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a986055f7db2

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a986055f7db2