Bug Fix - MiniSSL::Socket#write - use data.byteslice(wrote..-1) #2543
Conversation
Instead of data[wrote..-1]
| @@ -133,7 +133,7 @@ def write(data) | |||
|
|
|||
| return data_size if need == 0 | |||
|
|
|||
| data = data[wrote..-1] | |||
| data = data.byteslice(wrote..-1) | |||
There was a problem hiding this comment.
@MSP-Greg Do you understand why 5.1.1 doesn't seem to have the problem stated in #2542? This line of code wasn't changed in #2519 (but surrounding code was: https://github.com/puma/puma/pull/2519/files#diff-5f9e58f45420759185f3cff5203a523482b915cc3e331bb0f060977c2078d903)
There was a problem hiding this comment.
I will look at 5.1.1 and 5.2.0. I believe the bug requires a response with multi-byte characters, longer than 16kB, and ssl. And, since some html parsers can be rather forgiving...
There was a problem hiding this comment.
If I'm reading the code right, I think the bug was in changing line 133 from data.bytesize to data, no?
There was a problem hiding this comment.
Confused. Do you mean changing data.bytesize to data_size, which is set to the value of data.bytesize?
Anyway, I'm working thru it. The code in 5.1.1 was writing 512 byte chunks to the TCPSocket, which seemed not optimized. But, it also appears that it wrote all the data with @engine.write, where the current code is writing it in 16kB chunks. Because 5.1.1 was writing everything with @engine.write, maybe the issues with how it was looping weren't apparent?
Trying to figure out why the difference in write size is happening... Regardless, I believe the current code is a lot faster?
There was a problem hiding this comment.
I had to review the code changes in minissl.c, but I now recall. It's involved, but I'll write it up.
|
First, the question of why did Secondly, regarding the recent changes in Puma’s SSL for 5.2.0, I decided something was wrong while working on the new test framework, as it logged connection times. There was quite a large difference between TCP/Unix sockets and SSL sockets. Also, I was aware of 'Puma 4.3.6 - Rails 6.0.3.4 - TLS brings in memory leak'. That wasn’t ever really pinned down, but I thought it possible that a large number of open/idle ssl clients could use a fair bit of memory. Hence, the changes in 5.2.0. I changed the code to share an SSLContext, instead of creating a new one for each accepted connection, made some other adjustments to the Context, and re-wrote I thought this would be a reasonable implementation to both improve the speed and lower the memory. Testing locally, times have dropped for responses with both 10kB and 200kB bodies. The options chosen should reduce the memory required for open connections (keep-alive). I haven’t referenced OpenSSL man pages, but I (re-)read quite a few for the update… |
OK, now I get it |
Thanks for looking at it. I hope the explanation was reasonable, it's kind of messy... |
Instead of data[wrote..-1]
Description
MiniSSL::Socket#write performs some string 'dice & chop', and was using both byte and character methods. Convert all to byte methods.
Closes #2542
Note that this is only noticeable when the response headers/body are such that x.length != x.bytesize
I will write a test for it, but did the checking locally with a 600kB body. Was using
curlandgit diff, need to find a more Ruby based solution...Your checklist for this pull request
[changelog skip]or[ci skip]to the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.