Closed
Bug 1443853
Opened 6 years ago
Closed 6 years ago
Move geckodriver browser process shutdown code to mozrunner
Categories
(Testing :: Mozbase Rust, enhancement)
Testing
Mozbase Rust
Tracking
(firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
(Blocks 1 open bug)
Details
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jgraham
:
review+
|
Details |
geckodriver has some specialised code for safely waiting for the browser process to shut down in [1]. Notably this waits for the exception handler/watchdog in Firefox to interrupt the process if the shutdown hangs. It would make sense to uplift this to mozrunner as it is likely useful for other programs wrapping Firefox. It will also make MarionetteHandler::delete_session() easier to read. [1] https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/testing/geckodriver/src/marionette.rs#599-632
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8956982 [details] Bug 1443853 - Move browser process shutdown monitor to mozrunner. https://reviewboard.mozilla.org/r/225944/#review231938 ::: testing/geckodriver/src/marionette.rs:601 (Diff revision 1) > } > } > > - // If the browser is still open then kill the process > if let Some(ref mut runner) = self.browser { > - if runner.running() { > + // TODO: https://bugzil.la/1443922 Maybe explain the TODO a bit so we are not forced to always have to lookup the bug details. ::: testing/mozbase/rust/mozrunner/src/runner.rs:143 (Diff revision 1) > > + fn wait_for(&mut self, timeout: time::Duration) -> io::Result<process::ExitStatus> { > + match self.process.try_wait() { > + Ok(Some(status)) => Ok(status), > + Ok(None) => { > + debug!("Waiting {}s for browser to shut down", timeout.as_secs()); Just as a side-note... maybe we should do the same for the connection attempts? There is also a long list of entries. File a new bug for it if you think it is worth. ::: testing/mozbase/rust/mozrunner/src/runner.rs:157 (Diff revision 1) > + } > + > + if self.running() { > + warn!("Forcing browser process to shut down"); > + } > + self.kill() You only want to do this in the if condition.
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8956976 [details] Bug 1443853 - Remove unnecessary paranthesis around function argument. https://reviewboard.mozilla.org/r/225932/#review231962
Attachment #8956976 -
Flags: review?(james) → review+
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8956977 [details] Bug 1443853 - Drop unused std::ascii::AsciiExt trait. https://reviewboard.mozilla.org/r/225934/#review231964 BTW "Quences" isn't a word: https://en.oxforddictionaries.com/search?utf8=%E2%9C%93&filter=dictionary&query=Quences You probably mean "Quenches"?
Attachment #8956977 -
Flags: review?(james) → review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8956978 [details] Bug 1443853 - Rename RunnerProcess::is_running() to ::running(). https://reviewboard.mozilla.org/r/225936/#review231966
Attachment #8956978 -
Flags: review?(james) → review+
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8956979 [details] Bug 1443853 - Rename RunnerProcess::stop() to ::kill(). https://reviewboard.mozilla.org/r/225938/#review231986 Arguably this is wrong because it doesn't quite do the same thing as ::kill(). In particular it actually waits for the process to finish rather just signalling it. But I don't mind too much.
Attachment #8956979 -
Flags: review?(james) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8956980 [details] Bug 1443853 - Avoid std::io::{Result,Error} renaming. https://reviewboard.mozilla.org/r/225940/#review231988
Attachment #8956980 -
Flags: review?(james) → review+
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8956981 [details] Bug 1443853 - Rename RunnerProcess::status() to ::try_wait(). https://reviewboard.mozilla.org/r/225942/#review231990 ::: testing/geckodriver/src/marionette.rs:1362 (Diff revision 1) > - let status = runner.status(); > - if status.is_err() || status.as_ref().map(|x| *x).unwrap_or(None) != None { > + let exit_status = match runner.try_wait() { > + Ok(Some(status)) => Some( > + status > + .code() > + .map(|c| c.to_string()) > + .unwrap_or("signal".into()), At some point we should use `std::os::unix` to work out what the signal is in this case.
Attachment #8956981 -
Flags: review?(james) → review+
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8956981 [details] Bug 1443853 - Rename RunnerProcess::status() to ::try_wait(). https://reviewboard.mozilla.org/r/225942/#review231990 > At some point we should use `std::os::unix` to work out what the signal is in this case. I have written a crate, sysexit, which can do that. We can consider integrating it here.
Assignee | ||
Comment 16•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8956982 [details] Bug 1443853 - Move browser process shutdown monitor to mozrunner. https://reviewboard.mozilla.org/r/225944/#review231938 > Just as a side-note... maybe we should do the same for the connection attempts? There is also a long list of entries. File a new bug for it if you think it is worth. Yeah, I thought about that too. I’ve filed https://bugzil.la/1444068. > You only want to do this in the if condition. Calling ::kill() here is functionally equivalent to an if-condition call ::wait(), because ::kill() guarantees to return the same value when called a second time, and by this point the process should have exited.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8956982 [details] Bug 1443853 - Move browser process shutdown monitor to mozrunner. https://reviewboard.mozilla.org/r/225944/#review232100 ::: testing/geckodriver/src/marionette.rs:603 (Diff revision 2) > > - // If the browser is still open then kill the process > if let Some(ref mut runner) = self.browser { > - if runner.running() { > - info!("Forcing a shutdown of the browser process"); > - if runner.kill().is_err() { > + // TODO(https://bugzil.la/1443922): > + // Use toolkit.asyncshutdown.crash_timout pref > + if runner.wait_for(time::Duration::from_secs(70)).is_err() { I don't think `wait_for` is a good name because it doesn't tell me what's going on here; it sounds like a general condition mechanism (i.e. "just wait for 70s"). I see that the etomology is from `wait` in the stdlib, but I find this confusing. I would I think be happy with just `wait` but accepting a timeout or something more explicit. ::: testing/mozbase/rust/mozrunner/src/runner.rs:140 (Diff revision 2) > fn try_wait(&mut self) -> io::Result<Option<process::ExitStatus>> { > self.process.try_wait() > } > > + fn wait_for(&mut self, timeout: time::Duration) -> io::Result<process::ExitStatus> { > + match self.process.try_wait() { Why don't we just write this with the loop on the outside i.e. ``` let now = time::Instant::now(); while self.running() { if now.elapsed() > timeout { break; } thread::sleep(time::Duration::from_millis(100)); } if self.running() { self.kill() } else { self.try_wait() } ```
Attachment #8956982 -
Flags: review?(james) → review-
Assignee | ||
Comment 25•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8956982 [details] Bug 1443853 - Move browser process shutdown monitor to mozrunner. https://reviewboard.mozilla.org/r/225944/#review232100 > Why don't we just write this with the loop on the outside i.e. > > ``` > let now = time::Instant::now(); > while self.running() { > if now.elapsed() > timeout { > break; > } > thread::sleep(time::Duration::from_millis(100)); > } > if self.running() { > self.kill() > } else { > self.try_wait() > } > ``` This looks conceptually simpler, but has two problems: it makes it harder to atomically decide whether to log before waiting, and would force us to match on ::try_wait(). There are three variants we need to take care of from ::try_wait(): process has exited and returned with some exit status, process is still running, and an error occurred. The return type of this function is io::Result<process::ExitStatus>, which means it isn’t compatible with ::try_wait()’s io::Result<Option<process::ExitStatus>>. This is fixable by always calling self.kill() since this returns the return value of Child::wait(), and guarantees a consistent return value when called again. So if we are happy with dropping the logging I like this succinct approach better. I’ve uploaded an amended version for you to look at.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 33•6 years ago
|
||
mozreview-review |
Comment on attachment 8956982 [details] Bug 1443853 - Move browser process shutdown monitor to mozrunner. https://reviewboard.mozilla.org/r/225944/#review232594
Attachment #8956982 -
Flags: review?(james) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 41•6 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b6a8d0f3c88d Remove unnecessary paranthesis around function argument. r=jgraham https://hg.mozilla.org/integration/autoland/rev/985f63927da9 Drop unused std::ascii::AsciiExt trait. r=jgraham https://hg.mozilla.org/integration/autoland/rev/4988c81161f2 Rename RunnerProcess::is_running() to ::running(). r=jgraham https://hg.mozilla.org/integration/autoland/rev/1730b54d8ebf Rename RunnerProcess::stop() to ::kill(). r=jgraham https://hg.mozilla.org/integration/autoland/rev/83bb33caf3d3 Avoid std::io::{Result,Error} renaming. r=jgraham https://hg.mozilla.org/integration/autoland/rev/2da49990b247 Rename RunnerProcess::status() to ::try_wait(). r=jgraham https://hg.mozilla.org/integration/autoland/rev/a5d22f442915 Move browser process shutdown monitor to mozrunner. r=jgraham
Comment 42•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b6a8d0f3c88d https://hg.mozilla.org/mozilla-central/rev/985f63927da9 https://hg.mozilla.org/mozilla-central/rev/4988c81161f2 https://hg.mozilla.org/mozilla-central/rev/1730b54d8ebf https://hg.mozilla.org/mozilla-central/rev/83bb33caf3d3 https://hg.mozilla.org/mozilla-central/rev/2da49990b247 https://hg.mozilla.org/mozilla-central/rev/a5d22f442915
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•