fix(streaming): call stream.abort() explicitly when request is aborted #3042

usualoma · 2024-06-25T23:44:21Z

What is needed in bun?

In bun, when a ReadableStream is returned in the response, cancel() is not called if the connection is disconnected, so the hono needs to listen for the connection to be disconnected.

The following workaround is a strange piece of code, but I think it is acceptable because it would not work without it in bun.

https://github.com/honojs/hono/pull/3042/files#diff-b5c92850aac10e22281695a83a3d6d7a23f5455df59a11c597113914ef427774R14-R18

flaky test

Even stranger still, testing requests with Bun.serve() sometimes fails; I tried a number of patterns, including interspersing setTimeout, but I just couldn't get it to stabilise. The instability seems to occur only in unit tests, so if it succeeds on retry, it is considered OK.

0b15369

The author should do the following, if applicable

Add tests
Run tests
bun run format:fix && bun run lint:fix to format the code
Add TSDoc/JSDoc to document the code

…ort status

codecov · 2024-06-25T23:51:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.87%. Comparing base (a6ad42d) to head (49b770b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3042      +/-   ##
==========================================
+ Coverage   95.84%   95.87%   +0.03%     
==========================================
  Files         137      137              
  Lines       13428    13460      +32     
  Branches     2294     2282      -12     
==========================================
+ Hits        12870    12905      +35     
+ Misses        558      555       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

usualoma · 2024-06-28T00:08:37Z

@yusukebe Would you please review?

yusukebe · 2024-06-28T00:47:57Z

Hey @usualoma

I may not completely understand the correct behavior, but I think it does not work well in Bun with this PR.

Bun (does not work):

node.mp4

Node.js (works):

node.mp4

Btw, it has the same behavior as Node.js on Deno.

yusukebe · 2024-06-28T00:49:37Z

@usualoma

Is the abort fired when closing the browser?

yusukebe · 2024-06-28T01:23:56Z

Ah, we can use this script:

import { Hono } from 'hono'
import { streamSSE } from 'hono/streaming'
import { serve } from '@hono/node-server'

const app = new Hono()

app.get('t', (c) => {
  return streamSSE(c, async (stream) => {
    stream.onAbort(() => {
      console.log('aborted')
      stream.close()
    })
    for (;;) {
      try {
        await stream.writeSSE({
          data: `It is ${new Date().toISOString()}`,
          event: 'message',
        })
        await stream.sleep(1000)
      } catch (e) {
        console.error(e)
        break
      }
    }
  })
})

app.get('/', (c) => {
  return c.html(`
    <html>
      <body>
        <script>
const controller = new AbortController();
const signal = controller.signal;

const eventSource = new EventSource('/t', { signal: signal });

eventSource.onmessage = function(event) {
  console.log('Message:', event.data);
};

eventSource.onerror = function(error) {
  console.log('Error:', error);
  eventSource.close();
};

setTimeout(() => {
  console.log('Aborting connection');
  eventSource.close();
}, 5000);
        </script>
      </body>
    </html>
  `)
})

export default app
//serve(app)

But the behaviors are the same.

usualoma · 2024-06-28T01:28:13Z

@yusukebe Sorry, I think the stream() one is working, but the streamSSE() one was not. Please wait a bit while I fix it.

…, so hold it until the end of streaming

usualoma · 2024-06-28T03:39:53Z

@yusukebe
I finally understood what we needed.

In a bun, the destruction of c would start when the response was returned, so we needed to suppress that. In 55b6c8c, I made it so that c is held until a readableStream is destroyed, which solved the problem. This change also made flaky tests more stable.

yusukebe

LGTM!

yusukebe · 2024-06-28T22:00:24Z

Hi @usualoma !

Thanks for the investigation and implementation. it works well. Merging now.

usualoma added 3 commits June 26, 2024 08:24

feat(utils/stream): enable to abort streaming manually

c120338

feat(utils/stream): prevent multiple aborts, and enable to get the ab…

21c94b0

…ort status

fix(streaming): call stream.abort() explicitly when request is aborted

9f5a78c

usualoma force-pushed the feat/streaming-abort-bun branch from 20b94bc to 9f5a78c Compare June 25, 2024 23:47

usualoma mentioned this pull request Jun 26, 2024

Abort does not work with bun #3032

Closed

usualoma added 6 commits June 28, 2024 06:32

test: add tests for streaming

8426645

docs(stream): add comments

172fca2

test: add --allow-net to deno test command in ci.yml

9270399

Merge branch 'main' into feat/streaming-abort-bun

2bbe8b0

test(streaming): update test code

dce4b42

test(stream): retry flaky test up to 3 times at "bun"

0b15369

usualoma marked this pull request as ready for review June 28, 2024 00:08

test(streaming): refactor test to use afterEach

c43b89a

fix(streaming): in bun, c is destroyed when the request is returned…

55b6c8c

…, so hold it until the end of streaming

refactor(streaming): tweaks code layout

49b770b

usualoma mentioned this pull request Jun 28, 2024

Stream should not be closed automatically #2993

Closed

yusukebe approved these changes Jun 28, 2024

View reviewed changes

yusukebe merged commit 2d3bc55 into honojs:main Jun 28, 2024
14 checks passed

usualoma deleted the feat/streaming-abort-bun branch July 2, 2024 00:10

usualoma mentioned this pull request Jul 2, 2024

fix(stream): Fixed a problem that onAbort() is called even if request is normally closed in deno #3079

Merged

4 tasks

maehue mentioned this pull request Jul 25, 2024

[Snyk] Upgrade hono from 4.3.4 to 4.4.11 maehue/bun-hono-react-chat-app#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(streaming): call stream.abort() explicitly when request is aborted #3042

fix(streaming): call stream.abort() explicitly when request is aborted #3042

usualoma commented Jun 25, 2024 •

edited

Loading

codecov bot commented Jun 25, 2024 •

edited

Loading

usualoma commented Jun 28, 2024

yusukebe commented Jun 28, 2024

yusukebe commented Jun 28, 2024

yusukebe commented Jun 28, 2024

usualoma commented Jun 28, 2024

usualoma commented Jun 28, 2024

yusukebe left a comment

yusukebe commented Jun 28, 2024

fix(streaming): call stream.abort() explicitly when request is aborted #3042

fix(streaming): call stream.abort() explicitly when request is aborted #3042

Conversation

usualoma commented Jun 25, 2024 • edited Loading

What is needed in bun?

flaky test

The author should do the following, if applicable

codecov bot commented Jun 25, 2024 • edited Loading

Codecov Report

usualoma commented Jun 28, 2024

yusukebe commented Jun 28, 2024

yusukebe commented Jun 28, 2024

yusukebe commented Jun 28, 2024

usualoma commented Jun 28, 2024

usualoma commented Jun 28, 2024

yusukebe left a comment

Choose a reason for hiding this comment

yusukebe commented Jun 28, 2024

usualoma commented Jun 25, 2024 •

edited

Loading

codecov bot commented Jun 25, 2024 •

edited

Loading