Closed Bug 1413852 Opened 2 years ago Closed 2 years ago

TcpTransport.receive() doesn't obey current socket timeout

Categories

(Testing :: Marionette, defect)

58 Branch
defect
Not set

Tracking

(firefox57 fixed, firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(2 files)

The method TcpTransport.connect() is first connecting the socket with the default socket timeout of 360s. Then it tries to wait for a response from Marionette server but using a with statement, which itself changes the socket timeout:

https://dxr.mozilla.org/mozilla-central/rev/cd7217cf05a2332a8fd7b498767a07b2c31ea657/testing/marionette/client/marionette_driver/transport.py#195-198

The problem is that this sets the timeout directly on the socks instance but not for self.socket_timeout. As such receive() fails to take care of the shorter timeout because it uses the class' own property:

https://dxr.mozilla.org/mozilla-central/rev/cd7217cf05a2332a8fd7b498767a07b2c31ea657/testing/marionette/client/marionette_driver/transport.py#144

We should change the code to make all rely on the real sock timeout.

This change might fix a couple of outstanding intermittent issues, or brings us closer to the underlying issue.
If the 2s which are currently set to be used to receive the hello string are enough, is questionable. To prevent accidental socket timeouts for slower Firefox builds (debug, valgrind) I would suggest that we raise this timeout to at least 10s.
Attachment #8924495 - Flags: review?(ato)
Attachment #8924518 - Flags: review?(ato)
Attachment #8924495 - Flags: review?(mjzffr)
Attachment #8924518 - Flags: review?(mjzffr)
Comment on attachment 8924495 [details]
Bug 1413852 - TcpTransport.receive() doesn't obey current socket timeout.

https://reviewboard.mozilla.org/r/195770/#review201174
Attachment #8924495 - Flags: review+
Assignee: nobody → hskupin
Comment on attachment 8924518 [details]
Bug 1413852 - Improve failure message for socket connection attempts.

https://reviewboard.mozilla.org/r/195782/#review201176
Attachment #8924518 - Flags: review+
Attachment #8924495 - Flags: review?(mjzffr)
Attachment #8924495 - Flags: review?(ato)
Attachment #8924518 - Flags: review?(mjzffr)
Attachment #8924518 - Flags: review?(ato)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de33c2cf6efb
TcpTransport.receive() doesn't obey current socket timeout. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/06a8c0a33225
Improve failure message for socket connection attempts. r=automatedtester
https://hg.mozilla.org/mozilla-central/rev/de33c2cf6efb
https://hg.mozilla.org/mozilla-central/rev/06a8c0a33225
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1418227
You need to log in before you can comment on or make changes to this bug.