Closed Bug 1182681 Opened 10 years ago Closed 10 years ago

Raise IOError when we receive an empty string over the socket, and we're not tracking the gecko instance

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1166033 added support for failing fast when a Marionette socket request is pending and the underlying binary has died. Unfortunately, this broke wptrunner, because we no longer raise IOError when receiving an empty string on socket.recv().
Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham
Attachment #8632334 - Flags: review?(james)
Attachment #8632334 - Flags: review?(james)
Comment on attachment 8632334 [details] MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham https://reviewboard.mozilla.org/r/13041/#review11635 ::: testing/marionette/transport/marionette_transport/transport.py:61 (Diff revision 1) > # If we've launched the binary we've connected to, make This is fine, I guess but I'm still confused about what the case is where the connection is closed (i.e. the empty string is returned) but we want to keep polling for more data. Are we expecting the connection to be established at some point in the future? If that's the case, how did connect() succeed here? Or are we skipping the connect() call somehow (in which case it seems like the calling code is wrong).
Comment on attachment 8632334 [details] MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham https://reviewboard.mozilla.org/r/13041/#review11637 Ship It!
Attachment #8632334 - Flags: review+
We can get an empty string back while the process is in the midst of shutting down; keeping polling like this means we don't raise an IOError until the process has died, which means we won't try to restart it while the original process is still running. I can change the loop so that it just polls the instance in this case, rather than the socket + the instance.
Yeah I guess the structure I would prefer is while True: data = socket.recv() if not data: break response += data if have_full_response: return response if self.instance: self.instance.wait() raise IOError Although really it seems strange that the transport layer is in charge of managing the instance here.
Agreed; I'll look at moving that logic into the driver.
Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham
Attachment #8632868 - Flags: review?(james)
https://reviewboard.mozilla.org/r/13039/#review11727 ::: testing/marionette/driver/marionette_driver/marionette.py:695 (Diff revision 2) > + returncode = self.instance.runner.process_handler.wait() I think the runner itself has a .wait() method.
Comment on attachment 8632334 [details] MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham
Attachment #8632334 - Flags: review+ → review?(james)
Comment on attachment 8632868 [details] MozReview Request: Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham
Comment on attachment 8632868 [details] MozReview Request: Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham https://reviewboard.mozilla.org/r/13145/#review11733 Ship It!
Attachment #8632868 - Flags: review?(james) → review+
Comment on attachment 8632334 [details] MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham https://reviewboard.mozilla.org/r/13041/#review11767 Ship It!
Attachment #8632334 - Flags: review?(james) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acfcf574a1e9 This was missing re-raising the IOError in the case that self.instance was None. However it's also going to be necessary to update some wpt metadata because this patch also makes us correctly detect some crashes again (with the current behaviour on inbound they are incorrectly being seen as timeouts because we wait so long in the marionette driver). Ehsan: I don't know if it was obvious, but this is the bug to make quiting the browser after running a wpt actually quit the harness rather than hanging.
Attachment #8633554 - Flags: review?(jgriffin)
Attachment #8633554 - Flags: review?(jgriffin) → review+
Comment on attachment 8632334 [details] MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham * * * Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham
Attachment #8632334 - Flags: review+ → review?(james)
Attachment #8632868 - Attachment is obsolete: true
This looks good except for a perma-orange on OSX 10.8 which doesn't matter since we don't run 10.8 on trunk branches (not sure why we do on try...). I'll land if you give an r+ jgraham.
Attachment #8632334 - Flags: review?(james) → review+
Comment on attachment 8632334 [details] MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham https://reviewboard.mozilla.org/r/13041/#review11939 Ship It!
(In reply to James Graham [:jgraham] from comment #16) > Ehsan: I don't know if it was obvious, but this is the bug to make quiting > the browser after running a wpt actually quit the harness rather than > hanging. Awesome! Thanks for the explanation.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: