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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1413852 - TcpTransport.receive() doesn't obey current socket timeout.

The getter for socket_timeout should always return the current socket
timeout from the socket instance first, and only fallback to the
private property if no socket instance exists.

This ensures that all methods will always operate on the current
socket timeout value.

Also using a timeout of 2s for receiving the hello string might be too
less for slow running builds. To prevent intermittent failures for
start_session, a good value might be 60s.

Review commit: https://reviewboard.mozilla.org/r/195770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195770/

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1413852 - Improve failure message for socket connection attempts.

Instead of a general socket timeout failure indicate that no hello
data has been received through the socket right after the call to
connect().

Review commit: https://reviewboard.mozilla.org/r/195782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195782/

Comment 4

[David Burns :automatedtester]

Comment on attachment 8924495 [details]
Bug 1413852 - TcpTransport.receive() doesn't obey current socket timeout.

https://reviewboard.mozilla.org/r/195770/#review201174

Comment 5

[David Burns :automatedtester]

Comment on attachment 8924518 [details]
Bug 1413852 - Improve failure message for socket connection attempts.

https://reviewboard.mozilla.org/r/195782/#review201176

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ryan already pushed it even to beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/92ee9c76e7f8d61c39288f662d2e99b2ddf63c38
https://hg.mozilla.org/releases/mozilla-beta/rev/92ee9c76e7f8d61c39288f662d2e99b2ddf63c38

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/de33c2cf6efb
https://hg.mozilla.org/mozilla-central/rev/06a8c0a33225