-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Premature close #215
Comments
Could you upload an example to reproduce? Even better, would you like to send a PR to fix? |
Not an issue I will champion since my use case was addressed without using |
I got the same today. Having issues reproducing it, tho. Essentially same stack, but
|
Lmk if you can repro. |
@SimenB The way to reproduce is to start downloading a file and cancel the request while it is still buffering. Tip: using "3g slow" throttling in DevTools helps to replicate it. |
Thanks @gajus! I've put together https://github.com/SimenB/fastify-premature-close. However, the only way I get the error is if I use https://chrome.google.com/webstore/detail/imagus/immpkjjlgappgfkkfieppnmlhakdmaab?hl=en and hover the link to trigger a download, then move the cursor away. I recorded a quick screen capture to illustrate Screen.Recording.2022-03-25.at.16.25.06.movClicking the link and then aborting the download when it starts doesn't trigger the error, so it seems to be an issue when you abort before getting headers back, and not while the response is streaming? Not 100% sure. |
@mcollina are you able to reproduce? Might make sense to reopen the issue 🙂 |
The fix this would be to add an Lines 517 to 518 in 05811f9
There seem to be a very tight race condition. I would really like to have a unit test for this, I think we could test using proxyquire and overriding the dependency. Would you like to send a PR? |
Not sure I'm the correct person to fix this, streams are still somewhat magical to me 😅 What would the error handler do? Just swallow? |
I think just swallowing would be fine in this case. |
Seems to be Line 506 in 05811f9
peek call also makes no difference
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@mcollina thoughts on how to move forward here? I've suppressed the error from our logging now, but that means we might miss actual cases of bad stream handling, and not just these false positives. |
I'm currently focusing on shipping Fastify v4 and I don't have much time to dig deep into this problem. I'd be happy to take a PR that fixes it but it probably requires a couple of days of research to fix. |
Exciting! I don't know streams enough to properly debug this, so I'll wait for somebody else to have time. But no immediate rush 👍 |
Hey, I've forked your repo in order to reproduce the error directly with an http client ( after failing making a unit test showing the actual problem in the library): Yet, I've realized later that maybe it is not linked to the So, it seems to me more like a |
We get a lot of these two errors in production and are not able to reproduce it:
Since we use |
You are using |
I've tried using the reproduction from @SimenB with all the latest module versions and I can't reproduce. Could you verify? If the problem still appears, could you check if https://github.com/fastify/fastify-compress/tree/something solves it? I still need to finish this one but there should be enough features for you to verify. |
Thank you! I do not understand why hovering above the link should trigger a download. I could reproduce the error (with the versions used by @SimenB) by opening the link in a new tab and closing that tab. With
Edit: OK, I now installed the extension. But with the new versions, I still get the same |
You possibly need an |
I pushed 2 commits to my reproduction now - one upgrading to the latest versions of deps for fastify@3 (so moving to scoped packages, but not Note that if I use fastify@3 with the latest packages (manually editing From looking at https://github.com/fastify/fastify/releases/tag/v4.0.0-alpha.2, fastify/fastify#3651 seems like the obvious change which fixed this 🙂 |
Can confirm fastify/fastify#3651 fixed the issue - applying the |
Do you think we should backport that fix to v3.x? |
We would be glad since we use |
@TimotejR would you like to send a backport PR of fastify/fastify#3651 against https://github.com/fastify/fastify/tree/3.x? |
Yes! :) |
https://github.com/fastify/fastify/releases/tag/v3.29.1 has the backport 🎉 |
We still get the same errors. :( |
We would need a reproduction. |
Got this too. Solved by removing Error appeared after upgrading
and all current fastify components ("fastify-plugin" etc). No other code changes, so it worked fine with previous versions. Strange, the error never shown in production (on Heroku). Added a log just before the error, that shows, but then nothing on that request (which didn't complete). Only on development it was showing:
Trying to build a reproduction. |
Note that a logged error is the expected behavior as a connection was truncated. This issue was about an uncaught exception. |
Right. Saying that there was no logged error in production, just on development. It was hard to debug without a logged error. Reproduction:
misc.js 'use strict'
// eslint-disable-next-line require-await
export default async (fastify, opts) => {
fastify.get('/test', async (req, reply) => {
let page = fs.readFileSync(`./tpl/test.html`, 'utf8')
console.log('page', page)
reply.type('text/html') //otherwise it's plain text, instead of being interpreted as html
reply.send(page)
return
})
} |
Too late to the party, but Google sent me here, so I'll post a comment in case someone else would end up here as well.
So basically repace ...
reply.send(page)
return with return reply.send(page) After this, my requests started working, compression is working as well. |
We could finally reproduce the "error" by reloading the client during fetching a lot of data. I suggest to log a warning instead of an error. |
Its actually pretty easy to reproduce
Changing to |
fastify/fastify-compress#215 adding return to response.send
Fastify requires us to return the response object when handling async methods and dealing with exceptions, otherwise a thenable gets wrapped and improperly handled later causing an exception. This is shown by [this repo][1] and discuessed in [this issue][2]. [1]: https://github.com/sgrigorev/nestjs-compress-issue [2]: fastify/fastify-compress#215 (comment)
I am using reply.compress() directly, and I meet this error too. I can not explain why on the client side I got 'ERR_PADDING_2', I search for it, I found reference on nodejs on windows
at the end of the handler, fixed the problem. But really this happens only when using compress. Some combination I tried:
So, at the end I adopted the practice to return reply, that is ok every time. NOTE: my handler is async |
I can confirm returning even a basic JSON object will create this error unless you do as danielecr said above:
|
This is documented behaviour: https://fastify.dev/docs/latest/Reference/Routes/#promise-resolution |
i'm also getting this error with the direct JSON return pattern. i think it would be really nice if we could register an error handler for this case at the point that we register the plugin - that way we could choose to log it or ignore it ourselves. for now, we're going to change to use |
Please open a new detailed issue adding a Minimal, Reproducible Example or a feature request. |
Prerequisites
Fastify version
3.27.4
Plugin version
4.0.1
Node.js version
16.13.1
Operating system
macOS
Operating system version (i.e. 20.04, 11.3, 10)
12.0.1
Description
Steps to Reproduce
This appears to happen when request is aborted while still serving the file.
Expected Behavior
Should produce a warning, but not an uncaught error.
The text was updated successfully, but these errors were encountered: