Skip to content
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

Merged

Conversation

mashhurs
Copy link
Contributor

@mashhurs mashhurs commented Aug 16, 2022

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 sent 16367 bytes

payload = 'A' * (16 * 1024 + 123) # 16k + 123 = 16507

What 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 printfs to track the send size.

  • Config
output {
    stdout {}
    tcp {
        host => "google.com"
        port => 443
        ssl_enable => true
    }
}
  • Logs
Sending data: {"@timestamp":"2022-08-16T18:08:55.282249Z","event":{"original":"test"},"host":{"hostname":"localhost"},"@version":"1","message":"test"}
Sent size: 171

Sending data: {"@timestamp":"2022-08-16T18:09:04.404790Z","event":{"original":"generate"},"host":{"hostname":"localhost"},"@version":"1","message":"generate"}
Generating more than 16K payload...
Sent size: 16367
Sent size: 140 # expectation
  • Checked the thread no on the messages
warning: thread "[main]|output|tcp|client_socket-1" terminated with exception (report_on_exception is true):
  • Debugged logic
    • TCP server to test: gist
    • TCP client to test: gist

@mashhurs mashhurs marked this pull request as ready for review August 16, 2022 19:28
Copy link
Contributor

@yaauie yaauie left a 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 :)

lib/logstash/outputs/tcp.rb Outdated Show resolved Hide resolved
lib/logstash/outputs/tcp.rb Show resolved Hide resolved
lib/logstash/outputs/tcp.rb Outdated Show resolved Hide resolved
lib/logstash/outputs/tcp.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@yaauie yaauie left a 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 :)

lib/logstash/outputs/tcp.rb Outdated Show resolved Hide resolved
lib/logstash/outputs/tcp.rb Show resolved Hide resolved
Copy link
Contributor

@yaauie yaauie left a 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 👍🏼

lib/logstash/outputs/tcp.rb Show resolved Hide resolved
lib/logstash/outputs/tcp.rb Outdated Show resolved Hide resolved
lib/logstash/outputs/tcp.rb Show resolved Hide resolved
lib/logstash/outputs/tcp.rb Show resolved Hide resolved
lib/logstash/outputs/tcp.rb Outdated Show resolved Hide resolved
lib/logstash/outputs/tcp.rb Outdated Show resolved Hide resolved
@mashhurs
Copy link
Contributor Author

Tested after applying review feedbacks.

@mashhurs mashhurs requested a review from yaauie August 20, 2022 16:45
Copy link
Contributor

@yaauie yaauie left a 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 👍🏼

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants