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)
Remote Protocol
Marionette
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().
Assignee | ||
Comment 1•10 years ago
|
||
Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham
Attachment #8632334 -
Flags: review?(james)
Updated•10 years ago
|
Attachment #8632334 -
Flags: review?(james)
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
Agreed; I'll look at moving that logic into the driver.
Assignee | ||
Comment 7•10 years ago
|
||
Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham
Attachment #8632868 -
Flags: review?(james)
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
This broke web-platform-tests, so I backed it out:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5195daae919e
https://hg.mozilla.org/integration/mozilla-inbound/rev/84ffb30dd5b9
Comment 16•10 years ago
|
||
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.
Comment 17•10 years ago
|
||
Attachment #8633554 -
Flags: review?(jgriffin)
Assignee | ||
Updated•10 years ago
|
Attachment #8633554 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 18•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8632868 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
try run with these updated patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d52c59e0d5
Assignee | ||
Comment 20•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8632334 -
Flags: review?(james) → review+
Comment 21•10 years ago
|
||
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!
Comment 22•10 years ago
|
||
Comment 23•10 years ago
|
||
(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.
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6bf6fd025266
https://hg.mozilla.org/mozilla-central/rev/e667c2392aa8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•