node-mysql2 icon indicating copy to clipboard operation
node-mysql2 copied to clipboard

Fix backpressure when using TLS

Open vobarian opened this issue 2 years ago • 4 comments

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 https://github.com/sidorares/node-mysql2/issues/1134 Prior fix, which did not cover TLS, because the startTLS function was replacing the write method: handle backpressure when loading data from file https://github.com/sidorares/node-mysql2/pull/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.

vobarian avatar Dec 19 '22 22:12 vobarian

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

sidorares avatar Dec 20 '22 00:12 sidorares

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

vobarian avatar Jan 04 '23 15:01 vobarian

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.

tolgap avatar Sep 11 '23 21:09 tolgap

@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?

sidorares avatar Sep 12 '23 03:09 sidorares