fix: fix backpressure when using TLS #1752
Conversation
|
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: The Node.js docs state that Tls.TLSSocket was added in 0.11.4, therefore the |
|
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 |
|
@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
force-pushed
the
fix/tls-stream-backpressure
branch
from
37ca69a to
8ec58bc
Compare
|
Hello! I just revisited this, rebased, and added a couple tests. When using TLS, prior to the fix, these tests will show the problem by failing, and they pass with the fix in connection.js. I'm not entirely sure how the testing infrastructure works for this project, but I am assuming the tests will run with and without the MYSQL_USE_TLS=1 environment variable; they pass in both cases. |
|
Multiple CI jobs were failing due to the test not passing with a compressed connection. Backpressure for "load data infile" is not working with compression. This PR fixes backpressure with TLS but does not address the compression issue. Also, I may be misunderstanding the CI jobs, but I note there are multiple jobs with "Compression=0" in the name, but if you look at the stack trace for the errors (ERR_STREAM_WRITE_AFTER_END) it is clearly going through the Deflate functions in compressed_protocol.js. I don't know the details of how the tests and CI jobs work for this project but it sounds like compression is being enabled when it should not be. I briefly looked into the compression backpressure issue and it is not easy to fix. The basic problem is the length of the deflate queue is unbounded, so it can easily exhaust available memory and crash the process. Preventing that is complicated because it requires a mechanism to stop the writer and resume it when appropriate, but the way the compression code is written, it does not participate in the streams' queuing. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1752 +/- ##
==========================================
+ Coverage 89.23% 89.25% +0.02%
==========================================
Files 86 86
Lines 13583 13583
Branches 1585 1584 -1
==========================================
+ Hits 12121 12124 +3
+ Misses 1462 1459 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
test/integration/connection/test-backpressure-load-data-infile.test.cjs
Outdated
Show resolved
Hide resolved
test/integration/connection/test-backpressure-load-data-infile.test.cjs
Outdated
Show resolved
Hide resolved
test/integration/connection/test-backpressure-load-data-infile.test.cjs
Outdated
Show resolved
Hide resolved
wellwelwel
changed the title
…iously tests were incorrectly using compression because '0' as a string is truthy
…result set streaming test to show it works with compression; skip for MySQL < 8 since CTE isn't supported
test/integration/connection/test-backpressure-load-data-infile.test.cjs
Outdated
Show resolved
Hide resolved
test/integration/connection/test-backpressure-load-data-infile.test.cjs
Outdated
Show resolved
Hide resolved
|
Thanks, @vobarian! LGTM! |
…variations Co-authored-by: Weslley Araújo <[email protected]>
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.