Skip to content

More SSL_read() to fill big buffer #1454

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 16, 2023
Merged

More SSL_read() to fill big buffer #1454

merged 4 commits into from
May 16, 2023

Conversation

thuantr-dek
Copy link

@thuantr-dek thuantr-dek commented May 15, 2023 •
edited by azat
Loading

Once SSL_read() only get max 16K bytes (one TLS record).
In case of big buffer, should more SSL_read() to fill the buffer.

Fixes: #1446

@thuantr-dek thuantr-dek mentioned this pull request May 15, 2023
Closed
azat
azat reviewed May 15, 2023
View reviewed changes
Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I have few comments, please take a look

@azat
Copy link
Member

azat commented May 15, 2023

Once SSL_read() only get max 16K bytes (one TLS record).
In case of big buffer, should more SSL_read() to fill the buffer.

As far as I understand, the problem is that do_read simply does not use all space that io vectors has, so if SSL_read reads by 4K then it will not read all the data regardless of how much data is in the buffer (n_to_read), but simply will read 4K or 8K or data (depends on the number of io vectors will be allocated)

@thuantr-dek thuantr-dek force-pushed the improve-ssl-read branch 4 times, most recently from 7a21bac to cf39011 Compare May 15, 2023 13:08
azat
azat reviewed May 15, 2023
View reviewed changes
azat
azat reviewed May 15, 2023
View reviewed changes
@azat azat force-pushed the improve-ssl-read branch from cf39011 to 34189bb Compare May 15, 2023 19:20
azat
azat approved these changes May 15, 2023
View reviewed changes
Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@azat
Copy link
Member

azat commented May 15, 2023

By some reason http/https_openssl_basic started to fail with SSL_ERROR_SSL error

@azat
Copy link
Member

azat commented May 15, 2023 •
edited
Loading

Looks like SSL_OP_IGNORE_UNEXPECTED_EOF fixes the issue, but need to understand the problem

@thuantr-dek
Copy link
Author

Looks like SSL_OP_IGNORE_UNEXPECTED_EOF fixes the issue, but need to understand the problem

Any finding? I am trying to understand the test and failures...

@thuantr-dek
Copy link
Author

Please check latest update, seems verify OK now...

azat
azat reviewed May 16, 2023
View reviewed changes
azat
azat reviewed May 16, 2023
View reviewed changes
Thuan Tran and others added 4 commits May 16, 2023 21:11
@azat
Make bufferevent_set_max_single_read() effect
4ab3242
@azat
More SSL_read() to fill big buffer
ef51444 
Once SSL_read() only get max 16K bytes (one TLS record).
In case of big buffer, should more SSL_read() to fill the buffer.

Using sample https-client to measure max income MBit/s via nload tool.
Note: set bufferevent_set_max_single_read() by 32K and add the chunk
callback to read out each piece of data.

The client sample do https request a data 900KB (the server don't use
Transfer-Encoding: chunked)
- With origin/master: max income is 2.26 MBit/s
  The chunk callback never get a piece of data > 16K.
- With this PR: max income is 2.44 MBit/s
  The chunk callback can get some piece of data 32K or more.
@azat
Drop unused le_ssl_ops::err_is_ok
e5941b2 
It is not used anymore since errors are ignored if some progress had
been done.
@azat
ssl: add some comments for lack of notify_close
49a7ae4
@azat azat force-pushed the improve-ssl-read branch from fa2ad5d to 49a7ae4 Compare May 16, 2023 19:21
@azat azat merged commit 5324e48 into libevent:master May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azat azat azat approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How to increase max single read?
2 participants
@thuantr-dek @azat