sst / open-next

Open source Next.js serverless adapter
https://open-next.js.org
MIT License
3.7k stars 111 forks source link

Fix some cache issue #444

Closed conico974 closed 2 weeks ago

conico974 commented 2 weeks ago

Latest releases introduced multiple *-lite that replaced aws-sdk with aws4fetch. This introduced some bug and uncover some underlying issues with the incremental cache itself.

This PR fixes :

And the BIG one:

Cache set to S3 and sending to SQS were not properly awaited. Next itself doesn't seem to wait for cache set to be done before returning the response. This has been undetected because those call can achieve before the lambda returns (but no guarantee of that) and because subsequent call to the same lambda instance resumed the execution of those call. The sdk was able to handle restarting the request from scratch, or maybe failing silently in some cases, but aws4fetch wasn't able to handle this. This could also make some ISR call fails for up to 5min, for example if a revalidation request succeed, but the S3 PutObject failed the queue would not accept anymore request for this failed one for the 5 min deduplication window.

To fix this we used some detached promise along with AsyncLocalStorage to await everything we need before returning the response. This does not cause trouble for streaming return time ( at least in my account ) because it does not touch the stream, and it seems that they wait for the end of the handler function to end the lambda. Of course given aws records with streaming this could change, but there is a way around by awaiting those detached promises inside the _flush method of the OpenNextNodeResponse. This also means that we might have a way to handle next/after natively in lambda

changeset-bot[bot] commented 2 weeks ago

🦋 Changeset detected

Latest commit: a3bbef14b13007a9f4cdaa5dd5084bc524eba0dc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages | Name | Type | | ---------------- | ----- | | open-next | Patch | | app-pages-router | Patch | | app-router | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 2 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
open-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 18, 2024 9:33am
sommeeeer commented 2 weeks ago

good job

khuezy commented 2 weeks ago

Question: Since it now properly awaits everything before returning the response, is that a significant amount that affects the duration enough for people to care/notice?

conico974 commented 2 weeks ago

The next/after feature would be very useful for a lot of people.

The only thing with that is that it can only properly work with streaming. Hopefully aws can figure out all the streaming issues before Next 15 is out.

Since it now properly awaits everything before returning the response, is that a significant amount that affects the duration enough for people to care/notice?

Not really most people won't even notice a difference, the only things we await for now are the cache set (only for ISR/SSG) and the message sending to the queue. And those mostly happens not visible to the user with RefreshHit from cloudfront or during ISR calls. The only cases where this could be noticeable ( but still very minimal, like 10s of ms ) would be for page not on the CDN (first request or new page).

I'll run a few more test, but if everything is fine, i'll probably make a release for this early next week