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

Fix backpressure when using TLS #1752

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vobarian
Copy link

Rebase of PR #1559 which GitHub automatically closed :(

This PR fixes two related issues when using TLS:

Using LOAD DATA LOCAL, backpressure is not respected, causing out of memory errors if the data stream supplied by infileStreamFactory is faster than the upload to the database server
Using connection.query().stream(), backpressure is not respected if data is returned by the server faster than the client is handling it
Both issues result from the fact that the backpressure mechanisms of pause(), resume(), emit('pause'), on('drain'), etc., were being applied to the original stream object, which have no effect after that stream has been wrapped inside a TLSSocket, according to my experiments. This can lead to an out of memory error if the buffer grows too large.

See: backpressure not handled #1134
Prior fix, which did not cover TLS, because the startTLS function was replacing the write method: handle backpressure when loading data from file #1167

Note: There is still the legacy TLS support at the bottom of connection.js which I did not touch. The docs state TLSSocket was added in Node.js v0.11.4 which was released in 2013. It is so old I did not feel like looking into it.

@sidorares
Copy link
Owner

I'm happy to delete anything not required to support node v14+

@vobarian
Copy link
Author

vobarian commented Jan 4, 2023

I'm happy to delete anything not required to support node v14+

Sorry, my comment may have been confusing. I was just pointing out that the fix I created in this PR will only fix the problem for Node.js versions >= 0.11.4. Since you have stated you're not maintaining for those very old versions, this is fine and the fix can be merged.

It's not related to this PR, but if you want to clean up code that was for old Node versions, the if/else block at the bottom of connection.js can be deleted:

if (Tls.TLSSocket) {
  // not supported
} else {
  ...

The Node.js docs state that Tls.TLSSocket was added in 0.11.4, therefore the else never executes.
https://nodejs.org/dist/latest-v16.x/docs/api/tls.html#class-tlstlssocket

@tolgap
Copy link

tolgap commented Sep 11, 2023

Just to get some attention back to this 🙂

I am running into this issue. Right now I'm applying the git patch of this PR on each deployment for my application on the mysql2 package. Not ideal but it solves any OOM issues for me when using inlineStreamFactory with TLS.

@sidorares
Copy link
Owner

@tolgap sorry it didn't get enough attention. Unfortunately needs another rebase after the changes I added for bun compatibility. Would you be able to branch off vobarian:fix/tls-stream-backpressure, resolve conflicts, give it some testing and create fresh PR?

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.

None yet

3 participants