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

refactor: rewrite the net module to simplify state tracking #21244

Merged
merged 39 commits into from Nov 27, 2019

Conversation

@nornagon
Copy link
Member

nornagon commented Nov 21, 2019

Description of Change

This is a major rewrite of the net module. The main goals here are:

  1. Reduce complexity of state management. The existing URLRequest class has a lot of delicate logic around managing the lifetime of a request, which has subtle bugs. This refactor aims to substantially remove that state tracking through careful choice of interface.
  2. Move stream logic into JS. Working with JS streams in C++ is challenging. This moves all stream logic into JavaScript, simplifying the C++ code and replacing it with smaller, more modular and easier-to-understand JS.
  3. Make ClientRequest a real stream.Readable.

Checklist

Release Notes

Notes: Fixed some crashes that could occur when using the net module.

@nornagon nornagon added the wip label Nov 21, 2019
@nornagon nornagon requested review from zcbenz and deepak1556 Nov 21, 2019
@nornagon

This comment has been minimized.

Copy link
Member Author

nornagon commented Nov 21, 2019

TODO:

  • implement redirect logic
  • clean up 'abort' flow
  • write tests for upload body (POST/PUT) and redirect / auth. See #21200

Additionally, I'd like to expose some new features on this module, though that will come in a followup. Specifically:

  • ResourceRequestBody has support for directly uploading a file from the FS without going through JS. I'd like to expose that as net.request({ bodyFromFile: 'path/to/file' }).
  • This PR maintains the behavior that streaming data into the request buffers until the complete upload data is in memory before starting the request. This is because the full size of the stream must be known in advance. I'd like to introduce a change that will use proper streaming if the content-length header is set in advance.
@nornagon nornagon changed the title refactor: rewrite the `net` module to simplify state tracking refactor: rewrite the net module to simplify state tracking Nov 21, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 label Nov 22, 2019
nornagon added 2 commits Nov 22, 2019
@nornagon nornagon requested a review from electron/wg-upgrades as a code owner Nov 22, 2019
nornagon added 3 commits Nov 22, 2019
@nornagon

This comment has been minimized.

Copy link
Member Author

nornagon commented Nov 22, 2019

NB. this PR changes the way redirects are handled with net.request. Previously, there was an option to net.request called redirect, which could be "follow", "error" or "manual". We used that to fill the redirect_mode field of ResourceRequest.

However, that field is intended to be used for the fetch parameter of the same name (see here: https://fetch.spec.whatwg.org/#concept-request-redirect-mode) and doesn't work the way it was documented for net.request. Instead, I've replaced this with an API somewhat similar to the "login" event, where the default behavior if no listeners are registered is to follow redirects, but if a listener is registered then it's up to that listener to continue or cancel the request.

Copy link
Member

deepak1556 left a comment

👍

@nornagon nornagon merged commit d25256d into master Nov 27, 2019
18 of 20 checks passed
18 of 20 checks passed
electron-arm-testing Build #20191126.12 failed
Details
electron-arm-testing #20191126.12 failed
Details
Artifact Comparison Changes Detected
Details
Backportable? - 7-1-x Backport Failed
Details
Backportable? - 8-x-y Backport Failed
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-woa-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm64-testing Build #20191126.11 succeeded
Details
electron-arm64-testing #20191126.11 succeeded
Details
electron-woa-testing Build #20191126.11 succeeded
Details
electron-woa-testing #20191126.11 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Nov 27, 2019

Release Notes Persisted

Fixed some crashes that could occur when using the net module.

@nornagon

This comment has been minimized.

Copy link
Member Author

nornagon commented Nov 27, 2019

good luck trop

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 27, 2019

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 27, 2019

I was unable to backport this PR to "7-1-x" cleanly;
you will need to perform this backport manually.

@nornagon

This comment has been minimized.

Copy link
Member Author

nornagon commented Nov 27, 2019

          .--. .-,       .-..-.__
        .'(`.-` \_.-'-./`  |\_( "\__
     __.>\ ';  _;---,._|   / __/`'--)
    /.--.  : |/' _.--.<|  /  | |
_..-'    `\     /' /`  /_/ _/_/
 >_.-``-. `Y  /' _;---.`|/))))
'` .-''. \|:  .'   __, .-'"`
 .'--._ `-:  \/:  /'  '.\             _|_
     /.'`\ :;   /'      `-           `-|-`
    -`    |     |                      |
          :.; : |                  .-'~^~`-.
          |:    |                .' _     _ `.
          |:.   |                | |_) | |_) |
          :. :  |                | | \ | |   |
          : ;   |                |           |
          : ;   |                |   trop    |
          : ;   |                |           |
          : ;   |                | One patch |
          : ;   |                |  conflict |
        .jgs. : ;                | too many  |
-."-/\\\/:::.    `\."-._'."-"_\\-|           |///."-
" -."-.\\"-."//.-".`-."_\\-.".-\\`=.........=`//-".
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 27, 2019

@nornagon has manually backported this PR to "8-x-y", please check out #21303

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 27, 2019

@nornagon has manually backported this PR to "7-1-x", please check out #21304

@nornagon nornagon deleted the net-refactor branch Nov 27, 2019
zcbenz added a commit that referenced this pull request Nov 29, 2019
* refactor: rewrite the net module to simplify state tracking (#21244)

* fix build

* Update atom_api_net.cc
MarshallOfSound added a commit that referenced this pull request Dec 2, 2019
* refactor: rewrite the net module to simplify state tracking (#21244)

* un-ginify some things

* lint

* qualify util::Promise

* network::mojom::URLResponseHead -> network::ResourceResponseHead

* fix build

* Update api-net-spec.ts

* Update api-net-spec.ts

* Init -> InitWith
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.