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

Use a dedicated thread to buffer payloads, so avoiding deadlock #2480

Merged

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Feb 28, 2018

Fixes #2478

The problem is that sending a transaction first sends the payloads, then the transaction. This causes deadlock if sending the payload blocks, since the transaction is never sent, so the payloads are never removed from the buffer.

This PR fixes that, by adding a dedicated thread that routes from the payload IPC channel to a payload mpsc channel. It also removes a hot loop in the case of missing payloads.


This change is Reviewable

@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 28, 2018

r? @glennw

@glennw
Copy link
Member

glennw commented Feb 28, 2018

Some CI tidy failures:

./webrender_api/src/channel_ipc.rs:9: use statement is not in alphabetical order
	expected: std::{error, io}
	found: std::sync::mpsc
@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 28, 2018

@glennw fixed

@asajeffrey asajeffrey changed the title Use a dedicated thead to buffer payloads, so avoiding deadlock Use a dedicated thread to buffer payloads, so avoiding deadlock Feb 28, 2018
@glennw glennw self-requested a review Feb 28, 2018
@glennw
glennw approved these changes Feb 28, 2018
@glennw
Copy link
Member

glennw commented Feb 28, 2018

Looks good! Let's squash the commits and then this is ready to go.

…d by blocking ipc send
@asajeffrey asajeffrey force-pushed the asajeffrey:buffer-payloads-to-avoid-deadlock branch from 9a7b2d2 to 8bed771 Feb 28, 2018
@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 28, 2018

@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Feb 28, 2018

📌 Commit 8bed771 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Feb 28, 2018

Testing commit 8bed771 with merge f461a5b...

bors-servo added a commit that referenced this pull request Feb 28, 2018
…=glennw

Use a dedicated thread to buffer payloads, so avoiding deadlock

Fixes #2478

The problem is that sending a transaction first sends the payloads, then the transaction. This causes deadlock if sending the payload blocks, since the transaction is never sent, so the payloads are never removed from the buffer.

This PR fixes that, by adding a dedicated thread that routes from the payload IPC channel to a payload mpsc channel. It also removes a hot loop in the case of missing payloads.

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

bors-servo commented Mar 1, 2018

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: glennw
Pushing f461a5b to master...

@bors-servo bors-servo merged commit 8bed771 into servo:master Mar 1, 2018
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@kvark
Copy link
Member

kvark commented Mar 1, 2018

I wonder why we couldn't send the payload after transaction, which would also resolve the deadlock.

@asajeffrey
Copy link
Member Author

asajeffrey commented Mar 1, 2018

@kvark that would probably fix the problem, but a) it puts the WR thread into a hot loop until the payload arrives, and b) it tightly couples the sender and receiver which then have to agree about which is the current channel, which is rather brittle code. In the long run I think the plan is to move WR over to just using mpsc channels, and move any ipc over to the application.

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.

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