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

Support opt-in async network events #5156

Merged
merged 11 commits into from Apr 16, 2015
Merged

Support opt-in async network events #5156

merged 11 commits into from Apr 16, 2015

Conversation

@jdm
Copy link
Member

jdm commented Mar 5, 2015

This implements a framework for opting in to receiving network events asynchronously. It also converts XMLHttpRequest to use them, and paves the way for better support for synchronous XHR using on-demand, targeted event loops instead of spinning the global event loop. This gives us complete feature parity with the existing XHR implementation, using fewer threads than before in the async case.

Review on Reviewable

@highfive
Copy link

highfive commented Mar 5, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@jdm
Copy link
Member Author

jdm commented Mar 5, 2015

eb34a78 will be removed before merging; it's just an import of #5139.

@jdm jdm added the S-needs-rebase label Mar 10, 2015
@jdm
Copy link
Member Author

jdm commented Mar 10, 2015

@pcwalton Here's a smattering of relevant code to be evaluated for sandboxing concerns:
https://github.com/servo/servo/pull/5156/files#diff-3794c50e22356e01d16b66e3c3d15f46R227 - focus o n the new Listener branch here, as the Channel branch will be removed once all existing code is converted. We invoke a trait method in the privileged process which ends up sending the closure to the sandboxed process to be executed in the target task.
https://github.com/servo/servo/pull/5156/files#diff-a7309719ac3764589527303acba5a1bfR9 - the support code for passing events between the network thread in the privileged process and the target thread in the sandboxed process. Note that script_chan is just a trait object that provides an interface to send events to an event loop - this might be an existing channel, or a brand new one, as described in the next section.
https://github.com/servo/servo/pull/5156/files#diff-016eef304e69674de212b7fbcd9330fdR1042 - This is the code that initiates the request from the sandboxed process. The bit I'm worried about here in terms of sandboxing is in the handling of sync XHR - we create a new channel and make that the target of the async events instead of the regular existing script task event loop; for async requests we use the existing channels.

@jdm
Copy link
Member Author

jdm commented Mar 21, 2015

Huh, this never got a Critic review. I guess the review that's happening in #5197 can serve the same purpose.

@pcwalton
Copy link
Contributor

pcwalton commented Mar 31, 2015

I'm not sure about passing closures cross-process, in general, since closures aren't Encodable. But perhaps a code pointer plus an Encodable set of data that works like a closure would work.

@jdm jdm closed this Mar 31, 2015
@jdm jdm reopened this Mar 31, 2015
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Mar 31, 2015

Critic review: https://critic.hoppipolla.co.uk/r/4448

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@metajack
Copy link
Contributor

metajack commented Apr 1, 2015

@pcwalton has agreed to take this one.

@pcwalton
Copy link
Contributor

pcwalton commented Apr 1, 2015

Overall this looks pretty solid, but I'm not a big XHR expert. I'd like someone who's more intimately familiar with the XHR code, maybe @Manishearth, to take another look.

@Manishearth
Copy link
Member

Manishearth commented Apr 3, 2015

I'll have a look tomorrow.

@pcwalton pcwalton assigned Manishearth and unassigned pcwalton Apr 6, 2015
@pcwalton
Copy link
Contributor

pcwalton commented Apr 6, 2015

@jdm r=me with rebase

@jdm jdm force-pushed the jdm:asyncnet branch from bbe8bb1 to d0704d1 Apr 16, 2015
@jdm
Copy link
Member Author

jdm commented Apr 16, 2015

@bors-servo: r=pcwalton,Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2015

📌 Commit d0704d1 has been approved by pcwalton,Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2015

Testing commit d0704d1 with merge 3151497...

bors-servo pushed a commit that referenced this pull request Apr 16, 2015
This implements a framework for opting in to receiving network events asynchronously. It also converts XMLHttpRequest to use them, and paves the way for better support for synchronous XHR using on-demand, targeted event loops instead of spinning the global event loop. This gives us complete feature parity with the existing XHR implementation, using fewer threads than before in the async case.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5156)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit d0704d1 into servo:master Apr 16, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jdm jdm deleted the jdm:asyncnet branch Aug 4, 2015
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

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