search

sst / open-next

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

[Streaming] Lambda Timeout when using React Suspense #395

Closed lucasvieirasilva closed 2 months ago

lucasvieirasilva commented 3 months ago

In a regional deployment with streaming enabled, when the Next.js App uses React Suspense to stream the UI changes, the first chunk is received successfully in browser, but the following changes are not streamed by the lambda and after 10s the lambda timeout and the connection is closed in the browser.

I've added some debug logs to the responseStreaming adapter.

image

And after some investigation I've noticed that happened only when the this.responseStream.writableNeedDrain is true, which returns false to the socket write function.

When I return true regardless the this.responseStream.writableNeedDrain for my use case it always works.

I've also noticed that in this https://github.com/sst/open-next/pull/213/commits/d32d83d042c3590877c9fedf5a4a1deb90473f1b the return block was changed to return the value based on the this.responseStream.writableNeedDrain.

@khuezy I wonder why you had to do that, I'm assuming there's a reason for that, could you please tell me the reason?

I'm happy to raise a PR to make the return always true, but I'd like to understand the context first.

Thanks

khuezy commented 3 months ago

Hey @lucasvieirasilva thanks for investigating. To be honest I don't recall, we had to hack a bunch of stuff b/c AWS' streaming implementation has been poor since they released the feature. I hear they are aware of the issues and have a fix soon.

The streaming functionality on lambda seems to be very brittle, it may work in one scenario and break in others. Here's hoping they fix everything.

lucasvieirasilva commented 3 months ago

Hey @khuezy, yeah, and the AWS docs about it are not good as well, in the meantime, should we change the write function to always return true or leave it as is for now?

khuezy commented 3 months ago

I don't know what the state of the lambda runtime is at now. You can try to revert the change to return true and see how the e2e tests behave. iirc, the changed helped w/ the buffered response but that might have been a coincidence when I was testing.

conico974 commented 3 months ago

I don't think it's a good idea to touch that, at least not like that. To solve this we probably need to send a drain event. If we return true the stream will not be able to handle the backpressure. And because of how hacky the implementation is, it might fix something for you but break for someone else. Streaming is considered experimental in V2 and stuff are expected to break. Streaming in V3 is totally different than here, so everything done here is useless, and V3 is very close to being ready.

conico974 commented 2 months ago

Should be fixed in V3. Feel free to reopen if that's not the case