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

[Bug]: protocol.handle unexpected behavior when request.body is a ReadableStream #39658

Closed
3 tasks done
thecodrr opened this issue Aug 25, 2023 · 0 comments · Fixed by #41052
Closed
3 tasks done

[Bug]: protocol.handle unexpected behavior when request.body is a ReadableStream #39658

thecodrr opened this issue Aug 25, 2023 · 0 comments · Fixed by #41052
Labels
26-x-y bug 🪲 has-repro-gist Issue can be reproduced with code at https://gist.github.com/ platform/windows status/confirmed A maintainer reproduced the bug or agreed with the feature

Comments

@thecodrr
Copy link

thecodrr commented Aug 25, 2023

Preflight Checklist

Electron Version

26.1.0

What operating system are you using?

Windows

Operating System Version

Windows 11 22H2

What arch are you using?

x64

Last Known Working Electron version

No response

Expected Behavior

Iterating on a ReadableStream inside protocol.handle should not output anything except what is fed into the stream.

Actual Behavior

Currently, doing this outputs a bunch of undefined entries when it shouldn't

protocol.handle(SCHEME, async (request) => {
  console.log("starting stream");
  for await (const p of request.body) {
    console.log(p);
  }
  console.log("ending stream");
});

For some reason, the first few logs are correct outputting Uint8Array as expected but it eventually starts outputting undefined:

starting stream
Uint8Array(5) [ 84, 104, 105, 115, 32 ]
Uint8Array(3) [ 105, 115, 32 ]
Uint8Array(2) [ 97, 32 ]
Uint8Array(5) [ 115, 108, 111, 119, 32 ]
Uint8Array(8) [
  114, 101, 113,
  117, 101, 115,
  116,  46
]
undefined
undefined
undefined
undefined
undefined
undefined
undefined
undefined
ending stream

The main problem is that such a ReadableStream is not accepted by net.fetch or fetch and they crash with the following error:

node:52760) UnhandledPromiseRejectionWarning: TypeError [ERR_INVALID_ARG_TYPE]: The "chunk" argument must be of type string or an instance of Buffer or Uint8Array. Received undefined
    at new NodeError (node:internal/errors:399:5)
    at _write (node:internal/streams/writable:315:13)
    at ClientRequest.write (node:internal/streams/writable:337:10)
    at Object.write (node:internal/webstreams/adapters:179:63)
    at ensureIsPromise (node:internal/webstreams/util:192:19)
    at writableStreamDefaultControllerProcessWrite (node:internal/webstreams/writablestream:1115:5)
    at writableStreamDefaultControllerAdvanceQueueIfNeeded (node:internal/webstreams/writablestream:1230:5)
    at writableStreamDefaultControllerWrite (node:internal/webstreams/writablestream:1104:3)
    at writableStreamDefaultWriterWrite (node:internal/webstreams/writablestream:994:3)
    at [kChunk] (node:internal/webstreams/readablestream:1399:28)
(Use `Notesnook --trace-warnings ...` to show where the warning was created)
(node:52760) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 9)

On the browser side, I am doing a simple fetch like so:

  function wait(milliseconds: number) {
    return new Promise((resolve) => setTimeout(resolve, milliseconds));
  }

  const stream = new ReadableStream({
    async start(controller) {
      await wait(1000);
      controller.enqueue("This ");
      await wait(1000);
      controller.enqueue("is ");
      await wait(1000);
      controller.enqueue("a ");
      await wait(1000);
      controller.enqueue("slow ");
      await wait(1000);
      controller.enqueue("request.");
      controller.close();
    }
  }).pipeThrough(new TextEncoderStream());

const response = await fetch(uploadUrl, {
    method: "PUT",
    body: stream,
    signal,
    duplex: "half"
  });

Testcase Gist URL

https://gist.github.com/thecodrr/ddabb97be1e93435f9e25dcfab76876e

Additional Information

No response

@VerteDinde VerteDinde added platform/windows has-repro-gist Issue can be reproduced with code at https://gist.github.com/ 26-x-y labels Sep 4, 2023
@codebytere codebytere added the status/confirmed A maintainer reproduced the bug or agreed with the feature label Sep 6, 2023
BurningEnlightenment added a commit to BurningEnlightenment/electron that referenced this issue Jan 18, 2024
BurningEnlightenment added a commit to BurningEnlightenment/electron that referenced this issue Jan 18, 2024
zcbenz pushed a commit to BurningEnlightenment/electron that referenced this issue Feb 8, 2024
BurningEnlightenment added a commit to BurningEnlightenment/electron that referenced this issue Feb 14, 2024
jkleinsc pushed a commit that referenced this issue Feb 16, 2024
* refactor(protocol): extract file stream factory

Increase readability by moving the file stream creation logic out of the
`uploadData` to request body conversion function.

* fix: properly flatten streams in `protocol.handle()`

Refs: #39658

* fix: `protocol.handle()` filter null origin header

Refs: #40754

* fix: remove obsolete TODO comment

Refs: #38929

* fix: forward `Blob` parts in `protocol.handle()`

Refs: #40826

* fix: explicitly error out on unknown chunk parts
trop bot added a commit that referenced this issue Feb 16, 2024
Refs: #39658

Co-authored-by: Henrik S. Gaßmann <BurningEnlightenment@users.noreply.github.com>
trop bot added a commit that referenced this issue Feb 16, 2024
Refs: #39658

Co-authored-by: Henrik S. Gaßmann <BurningEnlightenment@users.noreply.github.com>
VerteDinde pushed a commit that referenced this issue Feb 19, 2024
* refactor(protocol): extract file stream factory

Increase readability by moving the file stream creation logic out of the
`uploadData` to request body conversion function.

Co-authored-by: Henrik S. Gaßmann <BurningEnlightenment@users.noreply.github.com>

* fix: properly flatten streams in `protocol.handle()`

Refs: #39658

Co-authored-by: Henrik S. Gaßmann <BurningEnlightenment@users.noreply.github.com>

* fix: `protocol.handle()` filter null origin header

Refs: #40754

Co-authored-by: Henrik S. Gaßmann <BurningEnlightenment@users.noreply.github.com>

* fix: remove obsolete TODO comment

Refs: #38929

Co-authored-by: Henrik S. Gaßmann <BurningEnlightenment@users.noreply.github.com>

* fix: forward `Blob` parts in `protocol.handle()`

Refs: #40826

Co-authored-by: Henrik S. Gaßmann <BurningEnlightenment@users.noreply.github.com>

* fix: explicitly error out on unknown chunk parts

Co-authored-by: Henrik S. Gaßmann <BurningEnlightenment@users.noreply.github.com>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Henrik S. Gaßmann <BurningEnlightenment@users.noreply.github.com>
mnvr added a commit to ente-io/ente that referenced this issue Apr 16, 2024
This was primarily for getting the latest Electron, but I ran `yarn
upgrade-interactive` and upgraded the other non-breaking deps (mostly dev) too.

Reason for wanting electron is to try and see if this backport fixes the issue
with our streams not getting faithfully written:
electron/electron#41052

In some ad-hoc and quick testing, I noticed that the new `writeStream` we've
implemented works fine for files up to 128 K, presumably some chunk size, but
then begins to diverge. Sounds similar (but not exactly the same) as this issue:
electron/electron#39658

Unfortunately, this didn't fix the issue we're facing, so our case is perhaps
different.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
26-x-y bug 🪲 has-repro-gist Issue can be reproduced with code at https://gist.github.com/ platform/windows status/confirmed A maintainer reproduced the bug or agreed with the feature
Projects
No open projects
Status: 👀 Unsorted Items
Development

Successfully merging a pull request may close this issue.

3 participants