-
Notifications
You must be signed in to change notification settings - Fork 32
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
Send entire payload considering IO#syswrite
return size.
#49
Send entire payload considering IO#syswrite
return size.
#49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that my first pass was effectively commenting only on lines that were whitespace-changes from the perspective of this PR, and haven't had time to fully grok the effective changes you're proposing. I'll get back to this tomorrow when my brain isn't a puddle of mush :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More discovery and feedback, now that my brain is back online :)
…ror, thread naming with atomic counter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has gone through a lot of iteration and discovery, but to my eyes it looks like it correctly:
- fixes an issue in both client and server where bytes from large payloads or to streams under back-pressure could be truncated
- fixes a regression from Feat: ssl_supported_protocols (TLSv1.3) #47 in which the logger method wasn't accessible to the client, so it would throw an error while attempting to log a closed stream.
- fixes an issue where client threads were not guaranteed to be distinct
I've left a few nitpicks, but it certainly looks on track to make things better 👍🏼
Tested after applying review feedbacks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion on the changelog to better describe the problem that we are fixing, but implementation and improvements all LGTM 👍🏼
Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>
Issue description
As a client, when sending over 16K data with SSL enabled mode, it is confirmed that some portion of data is not transferred.
Eg. generated
16507
bytes data,IO#syswrite
only sent16367
bytesWhat this PR does?
Takes in a count of
IO#syswrite
return size and iterates till payload is transmitted.Closes #41 #30 #33
Testing
Added a logic to generate over 16K data and put some
printf
s to track the send size.