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(WIP): support stream trailer #4373

Closed
wants to merge 2 commits into from
Closed

Conversation

climba03003
Copy link
Member

Support stream trailer trail PR.
I open this PR in WIP for collecting feedback. For example, is there any better solution?

Checklist

@climba03003 climba03003 marked this pull request as draft October 25, 2022 10:48
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

The change that makes trailer support promises and callbacks is very good. However, I don't think the sendStreamTrailer change is correct due the memory spike.

A memory friendly solution is to use https://github.com/mcollina/cloneable-readable to create a clone of the original stream and pipe one as a response and another to the trailer generation logic. This would also imply that we must call the trailer function immediately and not at the end of the response. Unfortunately, this would be a breaking change.

The good news is that once reply.trailer can be async, the stream support can be implemented in userland on top of it: create a clone of the stream in the main route handler and use that to compute the trailer value. This could be a good fit for a plugin:

const newStream = reply.wrapStreamWithTrailers(stream, {
  Etag: function (reply, payload, cb) {
    // fill in
    setImmediate(cb, null, '42')
  }
})
return newStream

lib/reply.js Outdated
// trailer attach should be fire immediately
// since Cloneable will block sending unless
// all the cloned copy is properly attached
sendTrailer(payload, res, reply)
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this is the breaking change I mentioned.

lib/reply.js Outdated Show resolved Hide resolved
lib/reply.js Outdated Show resolved Hide resolved
@climba03003

This comment was marked as resolved.

@climba03003 climba03003 mentioned this pull request Oct 26, 2022
4 tasks
lib/reply.js Outdated Show resolved Hide resolved
fastify.get('/', function (request, reply) {
reply.trailer('ETag', function (reply, payload, done) {
t.same(typeof payload.pipe === 'function', true)
payload.on('end', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

New issue comes, I have no way to know payload is ended when I use payload.pipe(res, { end: false }) in sendStream

lib/reply.js Show resolved Hide resolved

const _trailers = Object.entries(reply[kReplyTrailers]).filter(([k, v]) => typeof v === 'function')
Copy link
Member Author

Choose a reason for hiding this comment

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

is there good choice without using .filter?
The previous implementation using a single variables will count wrong and have racing condition.

cc @Uzlopak maybe you will have some idea?

Copy link
Member

Choose a reason for hiding this comment

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

You can implement the same with a for loop

@climba03003 climba03003 changed the base branch from main to next October 27, 2022 13:10
@climba03003
Copy link
Member Author

I am closing this PR since I don't have more time spent on it.
The major issue left in here is quite a critical one.

  1. When .pipe(stream, { end: false }) is used. The trailer handler will never know when the stream is ended.
  2. If we do not use clonable-readable, it face the memory issue.

I can give a hand if anyone would like to pick it up. Just ping me.

@climba03003 climba03003 closed this Nov 7, 2022
@climba03003 climba03003 deleted the feat-stream-trailer branch November 7, 2022 07:20
@mcollina
Copy link
Member

mcollina commented Nov 7, 2022

We have quite a few months before we can v5. Maybe you can revisit this in a few months?

@climba03003
Copy link
Member Author

climba03003 commented Nov 7, 2022

Maybe you can revisit this in a few months?

Yes.
The major issue is stuck in trailer handler have no way to know when the stream is ended.
I am not a stream expert and it seems a correct behavior in node since we use .pipe(stream, { end: false }).
When the user cannot attach .on('end', () => {}) to signal the callback is done. We are open in never ended request.

One solution is a timeout to check whether new content comes. But it is a hacky solution that I don't want to implement.

Copy link

github-actions bot commented Nov 8, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants