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

feat: add net module to utility process #40017

Merged
merged 30 commits into from Jan 4, 2024

Conversation

devm33
Copy link
Contributor

@devm33 devm33 commented Sep 28, 2023

Description of Change

Adds electron.net module to the utility-process. This enables making network calls from the utility process with the full capabilities of Chromium's native networking instead of the Node.js implementation. Example benefits include system TLS certificates, system proxy settings, support for basic, digest, NTLM, Kerberos, and negotiate authentication schemes.

The current net API in the main process requires any request from the utility process to be proxied via the main process UI thread. This PR makes it that main process establishes the URLLoaderFactory connection with the Network service and passes the remote endpoint to the utility process allowing it make requests directly with the Network service. This is borrowed on the same principle as renderer process perform today to make network requests.

Closes microsoft/vscode#192899
CC @deepak1556

Note: opening this with some remaining clean up work todo in order to get early feedback.

Remaining Completed clean up work:

Checklist

Release Notes

Notes: Added net module to utility process.

@welcome
Copy link

welcome bot commented Sep 28, 2023

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 28, 2023
@devm33 devm33 requested a review from a team as a code owner September 28, 2023 03:11
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't reviewed the content here but a question for folks with opinions @deepak1556 maybe.

I believe that shell/common is supposed to imply that code inside can run in any process, moving the net stuff into common would imply it would work in the renderer process which I don't think is correct 🤔 Thoughts on where this code should live?

@deepak1556
Copy link
Member

I couldn't suggest a better home with the current directory structure, but maybe following options can also be considered

  1. a component structure which allows sharing code between shell/browser and shell/services/node
  | components
    | net
      | js
        | net-client-request.ts
      | cpp
        | electron_url_loader.cc
        | electron_url_loader.h
      BUILD.gn
  | shell
     | browser
     | common
     | renderer
     | services
       | node
  | lib
    | browser
      | api
        | net.ts => depends on binding exposed from components/net and net-client-request.ts
    | common
    | renderer
    | utility
      | api
        | net.ts => depends on binding exposed from components/net and net-client-request.ts
  1. Keep things simple and allow utility to reuse code from browser, we already do this for message ports.

@devm33
Copy link
Contributor Author

devm33 commented Oct 18, 2023

Alright all of the planned follow items are completed here.

For PR description included and stakeholders cc'd, I'm not sure what groups or individuals should be cc'd. I think this requires API review, which looks like it's pending Semver Label Enforcement, but I'm not able to edit the labels here.

Any reviews or guidance through the process is much appreciated! 🙏

@dsanders11 dsanders11 added the semver/minor backwards-compatible functionality label Oct 18, 2023
@dsanders11
Copy link
Member

Some unrelated changed get pulled in to electron.d.ts due to the version bump of @electron/typescript-definitions. Perhaps that bump should land separately for a bit cleaner separation.

@devm33
Copy link
Contributor Author

devm33 commented Oct 18, 2023

Some unrelated changed get pulled in to electron.d.ts due to the version bump of @electron/typescript-definitions. Perhaps that bump should land separately for a bit cleaner separation.

Sure thing @dsanders11, opened #40264
Thanks for adding the semver label here!

@devm33
Copy link
Contributor Author

devm33 commented Oct 18, 2023

Some unrelated changed get pulled in to electron.d.ts due to the version bump of @electron/typescript-definitions. Perhaps that bump should land separately for a bit cleaner separation.

Sure thing @dsanders11, opened #40264 Thanks for adding the semver label here!

Oh it might be better left as part of this PR actually. Otherwise there are no utility process modules documented and the typescript definition generation fails.

@dsanders11
Copy link
Member

Oh it might be better left as part of this PR actually. Otherwise there are no utility process modules documented and the typescript definition generation fails.

Can @electron/typescript-definitions be fixed so that it doesn't fail in that case? Otherwise we'll be blocked if we need to land a new version of that package before this PR lands, and it may take a bit of time given the large nature of this PR.

@devm33
Copy link
Contributor Author

devm33 commented Oct 19, 2023

Can @electron/typescript-definitions be fixed so that it doesn't fail in that case? Otherwise we'll be blocked if we need to land a new version of that package before this PR lands, and it may take a bit of time given the large nature of this PR.

👍 On it!

@devm33
Copy link
Contributor Author

devm33 commented Oct 19, 2023

Can @electron/typescript-definitions be fixed so that it doesn't fail in that case? Otherwise we'll be blocked if we need to land a new version of that package before this PR lands, and it may take a bit of time given the large nature of this PR.

@dsanders11 opened electron/typescript-definitions#247!

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

shell/common/api/electron_api_url_loader.cc Show resolved Hide resolved
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

@jkleinsc
Copy link
Contributor

@deepak1556
Copy link
Member

If there are no other concerns, I plan to merge this PR today and follow-up on the system network context usage for net apis.

@jkleinsc jkleinsc merged commit 8c71e2a into electron:main Jan 4, 2024
17 checks passed
Copy link

release-clerk bot commented Jan 4, 2024

Release Notes Persisted

Added net module to utility process.

@trop
Copy link
Contributor

trop bot commented Jan 4, 2024

I have automatically backported this PR to "29-x-y", please check out #40890

@trop trop bot added in-flight/29-x-y and removed target/29-x-y PR should also be added to the "29-x-y" branch. labels Jan 4, 2024
@devm33 devm33 deleted the devm33/utility_net_api branch January 4, 2024 21:25
@trop trop bot added merged/29-x-y PR was merged to the "29-x-y" branch. and removed in-flight/29-x-y labels Jan 8, 2024
@miniak
Copy link
Contributor

miniak commented Jan 11, 2024

/trop run backport-to 28-x-y

@trop
Copy link
Contributor

trop bot commented Jan 11, 2024

The backport process for this PR has been manually initiated - sending your PR to 28-x-y!

@trop
Copy link
Contributor

trop bot commented Jan 11, 2024

I have automatically backported this PR to "28-x-y", please check out #40967

@trop
Copy link
Contributor

trop bot commented Jan 11, 2024

@miniak has manually backported this PR to "27-x-y", please check out #40968

miniak pushed a commit that referenced this pull request Jan 11, 2024
miniak pushed a commit that referenced this pull request Jan 11, 2024
jkleinsc pushed a commit that referenced this pull request Jan 18, 2024
feat: add net module to utility process (#40017)

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
@trop trop bot added merged/28-x-y PR was merged to the "28-x-y" branch. and removed in-flight/28-x-y labels Jan 18, 2024
jkleinsc pushed a commit that referenced this pull request Jan 18, 2024
feat: add net module to utility process (#40017)

Co-authored-by: Milan Burda <miburda@microsoft.com>
@trop trop bot added merged/27-x-y PR was merged to the "27-x-y" branch. and removed in-flight/27-x-y labels Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/approved ✅ merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore Electron.net API support in the utility process
8 participants