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 protocol.registerStreamProtocol #507

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@olizilla

olizilla commented Feb 23, 2018

Ports protocol.registerStreamProtocol from: electron/electron#11008

To land cleanly, that changeset requires other upstream changes to be ported:

bool Converter<const net::HttpResponseHeaders*>::FromV8

in: /electron/atom/common/native_mate_converters/net_converter.cc

The good news is that muon compiles with the ported changes, and using a fork of ipfs-companion I'm able to "stream" a response from ipfs via muon to brave.

screen shot 2018-02-23 at 16 43 46

I should qualify that I'm pretty good at spelunking around in codebases and porting things that look like they might work, but I have little C++ experience, so I'm offering this up as a starting point for discussion as a fix for brave/browser-laptop#10629

Also of note, I'd like to find a nicer way to stream the response over from the proxy impl of protocol.renderStreamProtocol in protocol_bindings.js, to the main process. I naively tried to use https://github.com/jprichardson/electron-ipc-stream but I'm not clear on the environment that the js in atom/common/resources/protocol_bindings.js is run in.

Streaming works sensibly now thanks to @alanshaw

olizilla added some commits Feb 22, 2018

alanshaw added some commits Mar 1, 2018

@alanshaw

This comment has been minimized.

alanshaw commented Mar 5, 2018

I've cleaned this up a little.

Likewise I have no experience with C so I can't really comment on that code. It could do with some 👀 from the muon team (and possibly some alterations?).

What's slightly bothering me atm is that the streaming works, but it doesn't support backpressure (notably neither does electron-ipc-stream). It means that if the destination starts saying "no more for now" we ignore it and keep pushing chunks through. I'm assuming that behind the scenes the extra chunks are buffered up until the destination is ready to receive again. I need to check that out. EDIT yes it just gets pushed onto an internal buffer

Pending the outcome on that we probably need to decide if that is a blocker or not. I'd like to register my preference to not block so we can get this landed and so people can start trying it out! 🚢 :shipit: 🚀 I have a feeling that implementing backpressure over an IPC channel might be difficult anyway.


As an FYI, streaming bit works like this:

We need to stream data from the renderer process to the main process. To do this we create a "cross process through stream", which is writable on the renderer side and readable on the main process side. When data comes in to the writable side on the renderer, we use the IPC channel to pass it over the boundary. The readable on the other side picks it up and makes it available to whoever is reading it.

In the renderer we're just attaching a 'data' event handler to the stream provided by the protocol handler and firing off an IPC message for each event.

In the main process there's now a readable IPC stream which listens to the IPC messages and pushes the data as it receives it, ignoring any backpressure signals from the destination.


Some other concerns:

We're sending chunks to the IPC channel in the order they arrive from the source stream. On the other side will they be fired as events in the same order? WE HAVE ASSUMED YES.

We added the tests from the electron PR but I couldn't figure out how to run them. Am I correct in saying that the tests aren't currently runnable in muon?

@olizilla olizilla changed the title from [WIP] Add protocol.registerStreamProtocol to Add protocol.registerStreamProtocol Mar 5, 2018

@@ -132,6 +116,22 @@ v8::Local<v8::Value> CreateFunctionFromTranslater(
v8::Object::New(isolate));
}
// func.bind(func, arg1).

This comment has been minimized.

@bridiver

bridiver Mar 10, 2018

Collaborator

does this have to move? It makes the change less clear

This comment has been minimized.

This comment has been minimized.

@bridiver

bridiver Mar 20, 2018

Collaborator

I see, ok

@@ -0,0 +1,44 @@
const Readable = require('stream').Readable

This comment has been minimized.

@bridiver

bridiver Mar 10, 2018

Collaborator

doesn't this file need to be added to the asar output in gn?

This comment has been minimized.

@olizilla

olizilla Mar 12, 2018

I don't know enough about the build process to answer to that. We took the existing deep-equal local dependency used by extensions.js as our guide. There's no reference to deep-equal in a gn file, so assumed we didn't need to.

This comment has been minimized.

@bridiver

bridiver Mar 20, 2018

Collaborator

sorry, I meant readable-ipc-stream.js, not require('stream')

This comment has been minimized.

@olizilla

olizilla Mar 28, 2018

@bridiver yep, that's what i figured you meant. I was trying to say that we looked at the use site over in https://github.com/tableflip/muon/blob/5c42c5d9755fef62518ae8a883e6ffb9aa8c20c4/lib/browser/api/extensions.js#L3-L6 and followed the pattern that was used for deep-equal which we didn't find referenced in a gn file.

@@ -93,6 +93,11 @@ class Protocol : public mate::TrackableObject<Protocol> {
net::URLRequestJob* MaybeCreateJob(
net::URLRequest* request,
net::NetworkDelegate* network_delegate) const override {
// hander not triggered if enabled.

This comment has been minimized.

@bridiver

bridiver Mar 20, 2018

Collaborator

?

This comment has been minimized.

@olizilla

olizilla Mar 28, 2018

Ah yes, that can go, it's just a left over from porting electron/electron#11008

@@ -0,0 +1,206 @@
// Copyright (c) 2017 GitHub, Inc.

This comment has been minimized.

@bridiver

bridiver Mar 20, 2018

Collaborator

can we make use of content::StreamURLRequestJob here?

This comment has been minimized.

@olizilla

olizilla Mar 28, 2018

I don't know enough about the codebase to answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment