Closed Bug 1094164 Opened 5 years ago Closed 5 years ago

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

Categories

(Testing :: Mozbase, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: zcampbell, Assigned: davehunt)

References

Details

Attachments

(1 file)

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
Blocks: 1038868
(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?
Flags: needinfo?(wlachance)
+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.
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.
Flags: needinfo?(wlachance)
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Attachment #8520049 - Flags: review?(wlachance)
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
Attachment #8520049 - Flags: review?(wlachance) → review+
https://hg.mozilla.org/mozilla-central/rev/a986055f7db2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.