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

Replaced mutex in constellation logging by a reentrant mutex. #12637

Merged
merged 1 commit into from Jul 29, 2016

Conversation

Projects
None yet
5 participants
@asajeffrey
Member

asajeffrey commented Jul 28, 2016

The double-panic in #12553 may be caused by using a non-reentrant lock, which panics on reetry. This PR adds a reentrant lock type (slightly annoyingly, the implementation in std isn't exported) and uses it for logging. cc @jdm


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #12619.
  • These changes do not require tests because they are designed to remove a class of intermittents.

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Jul 28, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@asajeffrey

This comment has been minimized.

Member

asajeffrey commented Jul 28, 2016

The unit tests are the same ones that are used by the Rust reentrant lock implementation.

@@ -0,0 +1,217 @@
/* This Source Code Form is subject to the terms of the Mozilla Public

This comment has been minimized.

@emilio

emilio Jul 28, 2016

Member

Is this true, is this source MPL?

This comment has been minimized.

@asajeffrey

asajeffrey Jul 28, 2016

Member

Well I wrote it :)

Actually, you have a point, which is that the unit tests are Apache/MIT licensed, not MPL. I'll fix that.

This comment has been minimized.

@emilio

emilio Jul 28, 2016

Member

Oh, I thought you copied it straight from libstd, but yeah, the tests still apply.

This comment has been minimized.

@asajeffrey

asajeffrey Jul 28, 2016

Member

No, it's the same interface as the libstd version, but implemented on top of std, rather than using reentrant locks from the OS.

// TODO: can we use the thread-id crate for this?

#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)]
pub struct ThreadId(NonZero<usize>);

This comment has been minimized.

@emilio

emilio Jul 28, 2016

Member

It would be nice to standardize this kind of things, we already have something like that in style/tid, would you mind filling a issue about this?

This comment has been minimized.

@Amanieu

Amanieu Jul 28, 2016

I would recommend using the thread-id crate instead since it does not make use of thread-local storage, which can be slow on some platforms.

This comment has been minimized.

@asajeffrey

This comment has been minimized.

@asajeffrey

asajeffrey Jul 28, 2016

Member

@Amanieu: I'd like to get this landed, then think about whether we can improve it by using third-party crates. Adding new dependencies to Servo is fairly easy, but not completely trivial.

@emilio

This comment has been minimized.

Member

emilio commented Jul 28, 2016

I don't think you need to ignore all the lints for the tests (see https://github.com/asajeffrey/servo/blob/3f2b55aa0b00d45c9bea873d55dc912298e17de0/python/tidy/servo_tidy/licenseck.py#L62).

LGTM, I'd like to review the mutex a bit more carefully, though in the meantime I guess we can give it a try run:

@bors-servo: try

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 28, 2016

⌛️ Trying commit 3f2b55a with merge 1606cd4...

bors-servo added a commit that referenced this pull request Jul 28, 2016

Auto merge of #12637 - asajeffrey:constellation-use-reentrant-logging…
…-mutex, r=<try>

Replaced mutex in constellation logging by a reentrant mutex.

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

The double-panic in #12553 may be caused by using a non-reentrant lock, which panics on reetry. This PR adds a reentrant lock type (slightly annoyingly, the implementation in std isn't exported) and uses it for logging. 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 #12619.
- [X] These changes do not require tests because they are designed to remove a class of intermittents.

<!-- 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/12637)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 28, 2016

💔 Test failed - mac-dev-unit

@asajeffrey

This comment has been minimized.

Member

asajeffrey commented Jul 28, 2016

http://build.servo.org/builders/mac-dev-unit/builds/2291 is failing because it's trying to build remutex (which uses unstable NonZero) for geckolib (which is stable).

@emilio

This comment has been minimized.

Member

emilio commented Jul 28, 2016

@bors-servo: try

LGTM with squash, let's see what try thinks :P

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 28, 2016

⌛️ Trying commit 22fe34c with merge 1e63e2f...

bors-servo added a commit that referenced this pull request Jul 28, 2016

Auto merge of #12637 - asajeffrey:constellation-use-reentrant-logging…
…-mutex, r=<try>

Replaced mutex in constellation logging by a reentrant mutex.

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

The double-panic in #12553 may be caused by using a non-reentrant lock, which panics on reetry. This PR adds a reentrant lock type (slightly annoyingly, the implementation in std isn't exported) and uses it for logging. 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 #12619.
- [X] These changes do not require tests because they are designed to remove a class of intermittents.

<!-- 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/12637)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

💔 Test failed - mac-rel-wpt

@asajeffrey asajeffrey force-pushed the asajeffrey:constellation-use-reentrant-logging-mutex branch from 22fe34c to e3030d0 Jul 29, 2016

@asajeffrey

This comment has been minimized.

Member

asajeffrey commented Jul 29, 2016

Squashed.

@asajeffrey

This comment has been minimized.

Member

asajeffrey commented Jul 29, 2016

I think the failure of /XMLHttpRequest/send-usp.worker is nothing to do with this PR, just checking on IRC to see if anyone's seen this before.

@asajeffrey

This comment has been minimized.

Member

asajeffrey commented Jul 29, 2016

@emilio

This comment has been minimized.

Member

emilio commented Jul 29, 2016

@bors-servo: r+

Yep, lgtm.

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

📌 Commit e3030d0 has been approved by emilio

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

⌛️ Testing commit e3030d0 with merge fdb1e51...

bors-servo added a commit that referenced this pull request Jul 29, 2016

Auto merge of #12637 - asajeffrey:constellation-use-reentrant-logging…
…-mutex, r=emilio

Replaced mutex in constellation logging by a reentrant mutex.

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

The double-panic in #12553 may be caused by using a non-reentrant lock, which panics on reetry. This PR adds a reentrant lock type (slightly annoyingly, the implementation in std isn't exported) and uses it for logging. 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 #12619.
- [X] These changes do not require tests because they are designed to remove a class of intermittents.

<!-- 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/12637)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

@bors-servo bors-servo merged commit e3030d0 into servo:master Jul 29, 2016

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