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

Only newlines send to clients. #260

Closed
florian-g2 opened this issue Sep 9, 2024 · 4 comments · Fixed by #261 or #263
Closed

Only newlines send to clients. #260

florian-g2 opened this issue Sep 9, 2024 · 4 comments · Fixed by #261 or #263
Labels
bug Something isn't working

Comments

@florian-g2
Copy link
Contributor

florian-g2 commented Sep 9, 2024

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

  • Version: 4.2.2
  • Platform: Amazon Linux 2023
  • Node.js Version: AWS Node 20 Runtime
@florian-g2 florian-g2 added the bug Something isn't working label Sep 9, 2024
@florian-g2
Copy link
Contributor Author

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

@H4ad
Copy link
Owner

H4ad commented Sep 9, 2024

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 added a commit to florian-g2/serverless-adapter that referenced this issue Sep 9, 2024
florian-g2 added a commit to florian-g2/serverless-adapter that referenced this issue Sep 9, 2024
Adds logic to ensure that the actual chunk is properly identified and passed into the streaming response.
@florian-g2
Copy link
Contributor Author

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
Copy link
Owner

H4ad commented Sep 9, 2024

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)

@H4ad H4ad linked a pull request Sep 9, 2024 that will close this issue
8 tasks
@H4ad H4ad closed this as completed in #261 Sep 9, 2024
@H4ad H4ad closed this as completed in 2aa474e Sep 9, 2024
H4ad added a commit that referenced this issue Sep 9, 2024
Improve the identification of chunks in streaming responses (Fixes #260)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants