Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add font cache debugging to isolate cause of IPC failures in CI. #20106

Merged
merged 1 commit into from Feb 22, 2018

Conversation

@jdm
Copy link
Member

jdm commented Feb 22, 2018

This should help us better understand the actual underlying cause of frequent CI failures like #13509. In particular, we will be able to state with confidence whether an IPC message is being dropped while the font cache thread is still alive, or whether the dropped sender was in a message that was in the queue after the font cache thread was shutdown.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes

This change is Reviewable

@highfive
Copy link

highfive commented Feb 22, 2018

warning Warning warning

  • These commits modify gfx code, but no tests are modified. Please consider adding a test!
@emilio
emilio approved these changes Feb 22, 2018
assert!(font_thread_has_closed, "Failed to receive a response from live font cache");
panic!("Font cache thread has already exited.");
}
let reply = reply
.expect("failed to receive response to font request");

This comment has been minimized.

Copy link
@emilio

emilio Feb 22, 2018

Member

nit: this now fits in one line.

This comment has been minimized.

Copy link
@emilio

emilio Feb 22, 2018

Member

Also, none of these need to use expect I guess, since we panic! on the line above... Want to either change it for unwrap(), or file an issue to revert this PR once we figure out the failures and reference it here?

assert!(font_thread_has_closed, "Failed to receive a response from live font cache");
panic!("Font cache thread has already exited.");
}
let reply = reply

This comment has been minimized.

Copy link
@emilio

emilio Feb 22, 2018

Member

same here.

assert!(font_thread_has_closed, "Failed to receive a response from live font cache");
panic!("Font cache thread has already exited.");
}
let instance_key = instance_key

This comment has been minimized.

Copy link
@emilio

emilio Feb 22, 2018

Member

Probably this too.

@jdm jdm force-pushed the jdm:font-thread-shutdown-debug branch 2 times, most recently from 5810cb5 to 192b64c Feb 22, 2018
@jdm
Copy link
Member Author

jdm commented Feb 22, 2018

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

bors-servo commented Feb 22, 2018

📌 Commit 192b64c has been approved by emilio

@highfive highfive assigned emilio and unassigned pcwalton Feb 22, 2018
@jdm
Copy link
Member Author

jdm commented Feb 22, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Feb 22, 2018

🔒 Merge conflict

@bors-servo
Copy link
Contributor

bors-servo commented Feb 22, 2018

The latest upstream changes (presumably #20021) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm jdm force-pushed the jdm:font-thread-shutdown-debug branch from 192b64c to 8d3b875 Feb 22, 2018
@jdm
Copy link
Member Author

jdm commented Feb 22, 2018

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

bors-servo commented Feb 22, 2018

📌 Commit 8d3b875 has been approved by emilio

@bors-servo
Copy link
Contributor

bors-servo commented Feb 22, 2018

Testing commit 8d3b875 with merge 267f9db...

bors-servo added a commit that referenced this pull request Feb 22, 2018
Add font cache debugging to isolate cause of IPC failures in CI.

This should help us better understand the actual underlying cause of frequent CI failures like #13509. In particular, we will be able to state with confidence whether an IPC message is being dropped while the font cache thread is still alive, or whether the dropped sender was in a message that was in the queue after the font cache thread was shutdown.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- 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/20106)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 22, 2018

@bors-servo bors-servo merged commit 8d3b875 into servo:master Feb 22, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.