Skip to content

Conversation

@vobarian
Copy link
Contributor

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.

@terrisgit
Copy link

terrisgit commented Sep 15, 2022

I seem to be in need of this. Any time @sidorares ? Found your sponsorship page. Wish I could do more but I won't stop it (sorry you'll have to wait to see that I'm not lying)

@sidorares
Copy link
Owner

LGTM

could you rebase on top of master @vobarian ? That should trigger workflows that were added after creation of this PR

@vobarian vobarian closed this Dec 19, 2022
@vobarian vobarian deleted the fix/tls-stream-backpressure branch December 19, 2022 22:07
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.

3 participants