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

Make sure fetch response body is read or cancelled during flush #18

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

athoscouto
Copy link

Not reading a fetch response can block the connection, making clients require multiple connections to keep working. In environments where the number of connections are limited, clients may experience random networking errors.

Not reading a fetch response can block the connection, making clients
require multiple connections to keep working. In environments where
the number of connections are limited, clients may experience random
networking errors.
}

await resp.body?.cancel();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to await?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should. This will make sure exceptions are properly propagated and that things are correctly cleaned up before we move forward.

Copy link

@haaawk haaawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should never happen right? We always have encoding equal to json or protobuf, no?

Copy link

@giovannibenussi giovannibenussi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me.

@athoscouto
Copy link
Author

This should never happen right? We always have encoding equal to json or protobuf, no?

libsql-server should always return one of the two indeed. But we have many proxies along the way that might return something else in case of errors

@athoscouto athoscouto merged commit cdd5126 into main Aug 15, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants