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

Implement {register,intercept}StreamProtocol #11008

Merged
merged 9 commits into from Nov 14, 2017

Conversation

Projects
None yet
8 participants
@tarruda
Contributor

tarruda commented Nov 3, 2017

This is an alternative to the existing functions in protocol module: Instead of returning buffer/string/filepath/httprequest, the user should return a node.js stream to provide the response body.

This new function is more flexible than the existing ones, since Files/HttpRequests/Buffers/Strings can all easily be converted into Streams using node.js standard API. Also, it should be more efficient than String/Buffers if the user wants to fetch data from the network.

Overview of changes:

  • Add infrastructure code to allow C++ classes to handle JS events.
  • Pass HTTP request headers to protocol handlers.
  • Implement {register,intercept}StreamProtocol (which also allows the user to return response headers)

While I'm still going to add tests, reviewers are welcome.

Close #4303 #8503

@tarruda tarruda requested review from zcbenz and deepak1556 Nov 3, 2017

@tarruda tarruda requested a review from electron/reviewers as a code owner Nov 3, 2017

@tarruda tarruda requested a review from electron/docs as a code owner Nov 10, 2017

@tarruda

This comment has been minimized.

Contributor

tarruda commented Nov 10, 2017

I'm having some trouble with documentation parsing, can someone help me?

@zeke

This comment has been minimized.

Member

zeke commented Nov 10, 2017

@tarruda it looks like the documentation is valid. What trouble are you having?

@tarruda

This comment has been minimized.

Contributor

tarruda commented Nov 10, 2017

@tarruda it looks like the documentation is valid. What trouble are you having?

True. When I ran npm lint locally, it failed when parsing documentation(some message about "headers" being already defined).

Apparently it is all fine, thanks anyway!

@zeke

This comment has been minimized.

Member

zeke commented Nov 10, 2017

I think @MarshallOfSound made some recent fixes to this. Your local checkout is probably slightly behind in its version of electron-docs-linter and/or electron-typescript-definitions.

# StreamProtocolResponse Object
* `statusCode` Number - The HTTP response code
* `headers` Object - A dictionary containing the response headers

This comment has been minimized.

@zeke

zeke Nov 10, 2017

Member

I would suggest replacing "dictionary" with "object", as the term "dictionary" is not really a part of the JS vernacular.

This comment has been minimized.

@tarruda

tarruda Nov 10, 2017

Contributor

👍

* `statusCode` Number - The HTTP response code
* `headers` Object - A dictionary containing the response headers
* `data` Stream - A node.js readable stream representing the response body

This comment has been minimized.

@zeke

zeke Nov 10, 2017

Member

Node.js should be capitalized

This comment has been minimized.

@tarruda

tarruda Nov 10, 2017

Contributor

👍

@zeke

zeke approved these changes Nov 10, 2017

I can't speak to the implementation, but the docs look good (aside from a few minor nits)

* `method` String
* `uploadData` [UploadData[]](structures/upload-data.md)
* `callback` Function
* `stream` (Stream | [StreamProtocolResponse](structures/stream-protocol-response.md)) (optional)

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Nov 11, 2017

Member

I'm assuming this is a ReadableStream, if so can we be explicit so I can add catches for streams in the typescript converter (we don't currently handle Streams)?

This comment has been minimized.

@tarruda

tarruda Nov 13, 2017

Contributor

👍

@deepak1556

This is great! Thanks for working on this. Can you run clang-format to fix some style issues ? LGTM otherwise 👍

namespace internal {
class EventSubscriberBase {

This comment has been minimized.

@deepak1556

deepak1556 Nov 13, 2017

Member

I think EventSubscriberBase can be moved to native-mate and can reuse some utilities from there, for example the CallbackHolderBase for JSHandlerData. What do you think ?

This comment has been minimized.

@tarruda

tarruda Nov 13, 2017

Contributor

I think EventSubscriberBase can be moved to native-mate

👍

and can reuse some utilities from there, for example the CallbackHolderBase for JSHandlerData. What do you think ?

Sure, but the main reason I implemented this class is because I couldn't make it work with the existing infrastructure. In particular, I tried to use the existing converters to pass a base::Callback to the javascript EventEmitter.on method but it didn't work as expected. IIRC it only executed the callback once.

I can try to sort through the issues and refactor to use more of the existing helpers but it will take some time. Can I do this (moving to native-mate + refactoring) after this PR is merged?

This comment has been minimized.

@deepak1556

deepak1556 Nov 13, 2017

Member

IIRC it only executed the callback once.

Yup, we should probably try to use the new base::Callback variant, base::RepeatingCallback. Can be done separately, not related to the PR.

I can try to sort through the issues and refactor to use more of the existing helpers but it will take some time. Can I do this (moving to native-mate + refactoring) after this PR is merged?

Sure, sounds good to me.

}
auto context = isolate->GetCurrentContext();
auto headers = v8::Local<v8::Object>::Cast(val);
auto keys = headers->GetOwnPropertyNames();

This comment has been minimized.

@deepak1556

deepak1556 Nov 13, 2017

Member

Its better to add a helper in mate::Dictionary to retrieve the keys and use it here instead. Can simply this function.

This comment has been minimized.

@tarruda

tarruda Nov 13, 2017

Contributor

👍

This comment has been minimized.

@tarruda

tarruda Nov 13, 2017

Contributor

Can you show an example of what you mean? I've tried using mate::Dictionary but it doesn't seem to provide any methods to retrieve all keys so I end up having to use something like headers.GetHandle()->GetOwnPropertyNames() (which would result in similar code).

This comment has been minimized.

@deepak1556

deepak1556 Nov 13, 2017

Member

Oh I meant adding a new method to mate::Dictionary that allows retrieving all the keys and work with it. I am fine with the existing converter but would be better if we can reuse stuff.

This comment has been minimized.

@tarruda

tarruda Nov 13, 2017

Contributor

Since this also involves sending a PR to another project, Can I do this in my next PR, after the necessary changes (EventSubscriberBase + Dictionary) are pulled into https://github.com/electron/native-mate?

@tarruda

This comment has been minimized.

Contributor

tarruda commented Nov 13, 2017

This is great! Thanks for working on this. Can you run clang-format to fix some style issues ? LGTM otherwise 👍

Can you help me? I tried ./vendor/depot_tools/clang-format but didn't work:

Problem while looking for clang-format in Chromium source tree:
  Could not find checkout in any parent of the current path.
@deepak1556

This comment has been minimized.

Member

deepak1556 commented Nov 13, 2017

For clang-format, I follow the steps described here https://github.com/electron/electron/blob/master/docs/development/clang-format.md. Hope that helps!

tarruda added some commits Nov 3, 2017

Stop allowing `electron.net` requests to be intercepted.
Not very useful to intercept requests from `electron.net`, since both the
interception machinery and `electron.net` live in the browser process.
Implement GetLoadTimingInfo in JsAsker class.
This is required to avoid a crash in blink when implementing a custom protocol
handler that deals with redirects.
Implement EventSubscriber<T> class.
This class simplifies the task of subscribing/handling javascript events from
C++ classes in the main process.
@tarruda

This comment has been minimized.

Contributor

tarruda commented Nov 13, 2017

@deepak1556 I have executed git-clang-format HEAD~1 into each individual commit while rebasing interactively, so the code formatting should be correct now.

tarruda added some commits Nov 3, 2017

Implement {register,intecept}StreamProtocol
These new functions are more flexible than the other
{intercept,register}*ProtocoProtocol functions, since it allows the user to
return a node.js stream to feed the data to the protocol handler.

It also allows the user to specify a response header dictionary, which makes it
possible to correctly intercept any request made from renderers.
@deepak1556

👍

@ckerr ckerr merged commit 31172ec into master Nov 14, 2017

7 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ckerr ckerr deleted the implement-stream-protocol branch Nov 14, 2017

olizilla added a commit to tableflip/muon that referenced this pull request Feb 22, 2018

@JstnPwll

This comment has been minimized.

JstnPwll commented Mar 8, 2018

@tarruda I'm using protocol.interceptStreamProtocol for HTTP requests. What I'd like to do is conditionally handle the request, reverting to the default handler if my conditions are not met. Is there any way to do this? I imagined I would be able to return true from my handler if I wanted to pass the request back to the default handler.

@tarruda

This comment has been minimized.

Contributor

tarruda commented Mar 8, 2018

@JstnPwll the decision to intercept the request is done in the IO thread before any javascript function is called in the UI thread. AFAIK the only way to do that would be to block the IO thread, ask javascript in the UI thread and then unblock the IO thread, but I think it is possible to cause deadlocks, so it is not a valid solution.

What you can do is to make a new request using the net.request API, and pass the response stream to the interceptStreamProtocol callback, this is what I've been doing.

It is not as simple as it sounds though, and I was planning to send a PR to add a new API that makes this easier, but didn't got the time to do it yet.

@JstnPwll

This comment has been minimized.

JstnPwll commented Mar 8, 2018

@tarruda Do you have some code you could share for using net.request inside the handler? I've got it partially working but I'm not sure what else I need to do to handle requests smoothly. For example, if I try to go to a Google login screen it seems to spin in an infinite loop (I thought it was redirecting but the response code was 200). Also certain stylesheets and images are failing.

@aishamidori

This comment has been minimized.

aishamidori commented May 17, 2018

@tarruda I'm running into issues trying to use interceptStreamProtocol from Typescript. Looks like it's currently typed as ReadableStream rather than NodeJS.ReadableStream? Is this intentional? The docs mention a Node.js readable stream and the example seems to pass one in.
https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream

@robjtede

This comment has been minimized.

robjtede commented Jul 22, 2018

@aishamidori It does seem like the typescript definitions are a bit off on this one. Using the JS ReadableStream in conjunction with this APIs is awkward. However, it appears that it isn't necessary. In the test files (https://github.com/electron/electron/blob/master/spec/api-protocol-spec.js#L872) they are using a node stream.PassThrough which does work.

There seem to be some issues with this APIs though. I'm having a hard time passing through both the data and headers.

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