Skip to content

fix: return promise from stream.cancel()#249

Closed
KnisterPeter wants to merge 2 commits into
honojs:mainfrom
KnisterPeter:fix-dangeling-promise
Closed

fix: return promise from stream.cancel()#249
KnisterPeter wants to merge 2 commits into
honojs:mainfrom
KnisterPeter:fix-dangeling-promise

Conversation

@KnisterPeter

Copy link
Copy Markdown

The promise returned from stream.cancel() was ignored so far. This lead to unhandled promises when the stream throws during the cancel operation. In turn the whole hono node-server was shutdown.

Close #248

@yusukebe

yusukebe commented Jul 4, 2025

Copy link
Copy Markdown
Member

Hi @KnisterPeter

Thank you for the PR. Can you add a test to confirm this change fixes the error?

@KnisterPeter

Copy link
Copy Markdown
Author

I will try my best 😃

The promise returned from stream.cancel() was ignored so far. This
lead to unhandled promises when the stream throws during the cancel
operation. In turn the whole hono node-server was shutdown.

Close honojs#248
@KnisterPeter
KnisterPeter force-pushed the fix-dangeling-promise branch from a6a70fa to b2c85f5 Compare July 14, 2025 11:31
@KnisterPeter

Copy link
Copy Markdown
Author

@yusukebe finally (after vacation) I managed to add a test case. Can you review it?

@KnisterPeter

Copy link
Copy Markdown
Author

@yusukebe I think the errors are not related to my changes. Can you confirm this?

@yusukebe

Copy link
Copy Markdown
Member

Hi @usualoma

Do you think this change is needed?

@usualoma

Copy link
Copy Markdown
Member

Hi @yusukebe
With the change in #258, cancel() is no longer called, so if the problem does not recur in @KnisterPeter's environment on v1.17.0, I think this PR is unnecessary.

If there are other issues, I think we need to consider another approach.

Hi @KnisterPeter
Could you please check how your environment has changed in v1.17.0?

@KnisterPeter

Copy link
Copy Markdown
Author

I will check and report back here.
Thanks for the explanation.

@KnisterPeter

Copy link
Copy Markdown
Author

@yusukebe @usualoma From our point of view the changes in #258 did fix our issue. Nice work 🥳 Thanks your for fixing it.
I'll close this PR and the issue by me.

@yusukebe

Copy link
Copy Markdown
Member

@KnisterPeter

Woooow. Great! Thank you!!

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.

Canceling an aborted stream terminates the hono process

3 participants