Skip to content

Conversation

@creativcoder
Copy link
Contributor

@creativcoder creativcoder commented Jul 20, 2016

An intermittent timeout was seen in #12516 , caused by panic of ServiceWorkerManager thread on reception of messages from resource thread. This PR amends things to not have the ServiceWorkerManager thread panic in such situation.
cc @jdm


  • These changes do not require tests because its an "intermittent fix"

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/serviceworker_manager.rs

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 20, 2016
@nox
Copy link
Contributor

nox commented Jul 20, 2016

Thanks for the contribution. I want a couple of changes but that's all.

-S-awaiting-review +S-needs-code-changes


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


a discussion (no related file):
It took me quite a bit of time to realise that it was about removing the two expect calls in receive_message.

Could you reword the commit message to make that crystal clear?


components/script/serviceworker_manager.rs, line 107 [r1] (raw file):

       while self.receive_message() {
        // process message
       }

Please restore the two additional handle_message methods and do something like this:

fn handle_message(&mut self) {
    while let Ok(message) = self.receive_message() {
        let should_continue = match message {
            Message::FromConstellation(msg) => {
                self.handle_message_from_constellation(msg)
            },
            Message::FromResource(msg) => {
                self.handle_message_from_resource(msg)
            }
        };
        if !should_continue {
            break;
        }
    }
}

components/script/serviceworker_manager.rs, line 160 [r1] (raw file):

        } else {
            panic!("Unexpected select result");
        }

I'm pretty sure you can continue to use select! here.


Comments from Reviewable

@nox nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 20, 2016
@nox nox assigned nox and unassigned emilio Jul 20, 2016
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 20, 2016
@creativcoder creativcoder changed the title do not panic sw-manager thread on resource recv error Remove expect calls in service worker manager thread Jul 20, 2016
@creativcoder
Copy link
Contributor Author

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.


a discussion (no related file):

Previously, nox (Anthony Ramine) wrote…

It took me quite a bit of time to realise that it was about removing the two expect calls in receive_message.

Could you reword the commit message to make that crystal clear?

Done

Comments from Reviewable

@nox nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 21, 2016
@nox
Copy link
Contributor

nox commented Jul 21, 2016

-S-awaiting-review +S-needs-code-changes

One last nit and I'll r+ it. Thanks again!


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/script/serviceworker_manager.rs, line 163 [r2] (raw file):

        select! {
            msg = msg_from_constellation.recv() => Ok(Message::FromConstellation(try!(msg))),
            msg = msg_from_resource.recv() => Ok(Message::FromResource(try!(msg)))

Nit: for these you can write msg.map(Message::FromConstellation) and msg.map(Message::FromResource).


Comments from Reviewable

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 21, 2016
@nox
Copy link
Contributor

nox commented Jul 21, 2016

bors-servo r+


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nox
Copy link
Contributor

nox commented Jul 21, 2016

...

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 66c04a0 has been approved by nox

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 21, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 66c04a0 with merge 07a0c2f...

bors-servo pushed a commit that referenced this pull request Jul 21, 2016
Remove expect calls in service worker manager thread

<!-- Please describe your changes on the following line: -->

An intermittent timeout was seen in #12516 , caused by panic of ServiceWorkerManager thread on reception of messages from resource thread. This PR amends things to not have the ServiceWorkerManager thread panic in such situation.
cc @jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix ##12516 (github issue number if applicable).

<!-- Either: -->
- [X] These changes do not require tests because its an "intermittent fix"

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12518)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

@bors-servo bors-servo merged commit 66c04a0 into servo:master Jul 21, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants