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

fix(ext/fetch): do not truncate field value in EventSource #22368

Merged
merged 1 commit into from Mar 25, 2024

Conversation

0f-0b
Copy link
Contributor

@0f-0b 0f-0b commented Feb 9, 2024

Depends on #22493. Closes #22367.

@crowlKats
Copy link
Member

could you add a test?

@0f-0b
Copy link
Contributor Author

0f-0b commented Feb 9, 2024

EventSource reestablishes the connection if the response stream ends, but the test fails if I add a call to eventSource.close() to prevent this behavior.

running 1 test from ./cli/tests/unit/event_source_test.ts
eventSourceColonInMessage ...
Uncaught error from ./cli/tests/unit/event_source_test.ts FAILED
eventSourceColonInMessage ... cancelled (0ms)

 ERRORS 

./cli/tests/unit/event_source_test.ts (uncaught error)
error: (in promise) AbortError: The signal has been aborted
  eventSource.close();
              ^
    at AbortSignal.[[[signalAbort]]] (ext:deno_web/03_abort_signal.js:135:14)
    at AbortController.abort (ext:deno_web/03_abort_signal.js:280:30)
    at EventSource.close (ext:deno_fetch/27_eventsource.js:186:28)
    at eventSourceColonInMessage (file://.../deno/cli/tests/unit/event_source_test.ts:28:15)
This error was not caught from a test and caused the test runner to fail on the referenced module.
It most likely originated from a dangling promise, event/timeout handler or top-level code.

 FAILURES 

./cli/tests/unit/event_source_test.ts (uncaught error)

FAILED | 0 passed | 2 failed (36ms)

error: Test failed

I'll try to fix this in another PR.

@0f-0b 0f-0b force-pushed the fix-eventsource-truncation branch 2 times, most recently from fe914fb to 0662c23 Compare February 10, 2024 20:29
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Looks good to me! Let's merge this after the linked robustness PR.

Thank you for the contributions <3

@bartlomieju bartlomieju enabled auto-merge (squash) March 10, 2024 22:40
@cowboyd
Copy link

cowboyd commented Mar 18, 2024

Is there a way to consume this fix from Deno 1.41.3 in order to work around this issue until there is an official release?

@cowboyd
Copy link

cowboyd commented Mar 18, 2024

Is there a way to consume this fix from Deno 1.41.3 in order to work around this issue until there is an official release?

I found this polyfill for Node which works well with just:

import EventSource from "npm:eventsource";

@0f-0b
Copy link
Contributor Author

0f-0b commented Mar 24, 2024

@bartlomieju, please take a look at #22493 first. Testing this fix requires there being no unhandled rejections or resource leaks.

crowlKats pushed a commit that referenced this pull request Mar 24, 2024
This PR fixes all unhandled rejections and resource leaks found while
adding a test for #22368.
auto-merge was automatically disabled March 24, 2024 17:50

Head branch was pushed to by a user without write access

@0f-0b 0f-0b force-pushed the fix-eventsource-truncation branch from 8484add to 324583d Compare March 24, 2024 17:50
Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM

@crowlKats crowlKats merged commit 5c1fa0c into denoland:main Mar 25, 2024
17 checks passed
@0f-0b 0f-0b deleted the fix-eventsource-truncation branch March 25, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deno does not deliver all the information with the EventSource
5 participants