H4ad / serverless-adapter

Run REST APIs and other web applications using your existing Node.js application framework (NestJS, Express, Koa, tRPC, Fastify and many others), on top of AWS, Azure, Huawei and many other clouds.
https://serverless-adapter.viniciusl.com.br/
MIT License
134 stars 8 forks source link

Only newlines send to clients. #260

Closed florian-g2 closed 1 month ago

florian-g2 commented 1 month ago

Hi! First: Thank you for your work on this library. It is a blessing that I can use it as a drop-in replacement for the vendia / codegenie serverless express for leveraging streaming responses in aws.

Current Behavior

I encountered a bug where only some new lines (\r\n) were send to the client with the AwsStreamHandler and the express framework. It turned out that something with the writesToIgnore counting in response-stream.ts is off. Patching the library with this workaround prevents the issue:

if (!isFirstCall && internalWritable) {
...
    if (encoding === undefined) {
        log.debug('SERVERLESS_ADAPTER:RESPONSE_STREAM:MY_BYPASS');
        internalWritable.write(data, cb);
        return true;
    }
...
}

In my case I saw using the AWS Console that my desired write had the encoding set to undefined. The other writes had the encoding set to null or latin1. Therefore this workaround worked for me.

Expected Behavior

The correct data is send to the client.

Steps to Reproduce the Problem

Sorry, I'm currently short on time in my project, so I can't provide a proper reproduction. I have a Screenshot of my AWS console though (pre-workaround) and I have a possible suspect. The only thing that I use in my code which is not that common is probably the response.flushHeaders(); call before sending any data. Else I only call .write() and .end() on the express response. Other routes which do not use response.flushHeaders(); work as expected.

Screenshot: image

Environment

florian-g2 commented 1 month ago

I might have a PR ready soon. I needed to dig into it. My workaround was not quite sufficient.

H4ad commented 1 month ago

I have do to some magic around the headers to be able to intercept, mount the request with the headers and then start streaming the data. Why do you need to call flushHeaders? Can you remove this method call?

Also, if you have time, try to create a test like this one https://github.com/H4ad/serverless-adapter/blob/main/test/handlers/aws-stream.handler.spec.ts#L47-L84 to be able to reproduce this issue.

What http framework are you using?

florian-g2 commented 1 month ago

Well well, my push history is visible here, yeah I failed to read that the conventional commits are required here. 😆

Why do you need to call flushHeaders?

My service provides some sort of "download xyz" route which a browser can call. The source where the data is read from is quite slow, so I flush the headers so that the browser can notice the Content-Disposition header and display a download popup.

Also, if you have time, try to create a test

I added a test in my PR which just adds a .flushHeaders() call. The test fails with the current implementation.

What http framework are you using?

Express. Maybe this is not relevant, .flushHeaders() is node build-in.

Let's continue in the PR. I add some other comments there later. Need to watch some infamous event now.

H4ad commented 1 month ago

so I flush the headers so that the browser can notice the Content-Disposition header and display a download popup.

This doesn't work because AWS has some issues with buffering, so only after sending 16kb of data it will actually send the data to the client (ref)