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

feat: implement stream.onAbort #1795

Closed
wants to merge 6 commits into from

Conversation

sor4chi
Copy link
Contributor

@sor4chi sor4chi commented Dec 9, 2023

ref: #1770

Author should do the followings, if applicable

  • Add tests
  • Run tests
  • yarn denoify to generate files for Deno

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 9, 2023

Hi, @yusukebe
Here is the POC as of now for now.
I tried to implement it with as few destructive changes as possible, but it looks like I need to change the StreamAPI a bit anyway.
The fact that the test is failing shows that there is a lot of difference in runtime.

@yusukebe
Copy link
Member

yusukebe commented Dec 9, 2023

@sor4chi

Thanks! I'll check this later!

@yusukebe
Copy link
Member

Hi @sor4chi

This is a great work! It works fine on Node.js and Deno.

One thing you need to consider is that this behavior is changed from the current release version.

The code:

app.get('/stream', (c) => {
  return c.streamText(async (stream) => {
    for (;;) {
      await stream.writeln('Hello')
      console.log('Hello')
      await stream.sleep(1000)
    }
  })
})

The current version:
https://github.com/honojs/hono/assets/10682/60fc7e86-13c3-438a-a41a-587a076ad2e5

This PR:
https://github.com/honojs/hono/assets/10682/a9269705-03ab-4891-91c0-a4779adf41d9

Could you take a look?

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 11, 2023

@yusukebe
Wow, there was such a bug. I was not aware of that, thank you.

I will certainly have to fix this, but it will take some time.

@HeyITGuyFixIt
Copy link
Contributor

@sor4chi @yusukebe any updates?

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 18, 2023

@HeyITGuyFixIt
No, I'm Sorry.
I'm too busy and don't have time to work.

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 23, 2023

waiting for #1846

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 28, 2023

Hi, @yusukebe
I have investigated this and I believe that the behavior up to now was a bug in the first place.
Do you have any concerns about this change to a specification that interrupts the stream in the middle?

@yusukebe
Copy link
Member

@sor4chi

Do you have any concerns about this change to a specification that interrupts the stream in the middle?

I do not know. I can look into it myself, but I would prefer to leave it to you.

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 28, 2023

As for the change that writing to the readable stream is terminated in the middle and no further processing is performed, I would like to proceed as is because the original implementation was poor and I think that changing this behavior in the first place will have little effect on users.

@yusukebe
Copy link
Member

@sor4chi

Okay! Let's go with this spec, which is implemented in this PR.

@sor4chi sor4chi marked this pull request as ready for review December 28, 2023 15:45
@yusukebe yusukebe changed the base branch from main to next December 29, 2023 12:50
@yusukebe
Copy link
Member

@sor4chi

Since this referenced the old code, can you create a new PR based on the "next" branch?

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 29, 2023

@yusukebe
oh, sorry. Okay!

@sor4chi sor4chi closed this Dec 29, 2023
@sor4chi sor4chi mentioned this pull request Dec 29, 2023
3 tasks
@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 29, 2023

@yusukebe
this is the new pull request. #1871

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.

3 participants