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

Workaround asio::ssl async_read_some busy-loop #69

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

adamncasey
Copy link
Contributor

@adamncasey adamncasey commented Mar 25, 2022

asio::ssl has a bug where calling async_read_some with null_buffers
isn't correctly handled, and more or less immediately invokes the
provided handler with no data. This causes amqpprox to busy-loop
whenever a TLS socket has been accepted or opened.

There were two options for how to address this in our code:

  1. Always read with a fixed size buffer, such as 32kb. This would
    simplify the code slightly, at the expense of needing multiple reads
    to handle larger than 32kb frames in non-TLS mode, even when the full
    frame is available to amqpprox in one go.
  2. Ask asio::ssl to read with a very small buffer, then ask the openssl
    library how many bytes are available. This technique aligns with how
    amqpprox's read loop works today. That is what is implemented here.

In theory something similar could be upstreamed into asio::ssl. It's a
little tricky though and this exact code couldn't handle the generic
MutableBufferSequence interface - we can take some shortcuts in our
code.

I've done some benchmarking to check this change isn't going to regress
performance noticeably. Data throughput tests indicate that this fix improves
performance for TLS connections over the existing code.

Performance Tests

Testing by setting up amqpprox as per the performance tests README.md, and running the perf tester like:

$ ./amqpprox_perf_tester --address amqp://localhost:30671 --listen-address 0.0.0.0:5671 --clients 10 --max-threads 10 --message-size 10000000 --num-messages 100 --listen-cert amqpprox/cert.pem --listen-key amqpprox/key.pem

Results in MiB/s over a ~20second run

Test # no TLS no TLS no TLS TLS TLS TLS
Code Current Fixed Buffer 32K This PR Current Fixed Buffer 32K This PR
1 898 1038 824 426 515 505
2 965 902 1007 470 535 514
3 1050 888 918 475 553 555

This shows there isn't really any chance to non-TLS performance, and there is a slight increase in throughput with this code change (or the fixed-buffer size change).

Still TODO

  • Check against a few rabbitmq client libraries. Ensure I'm not missing something obvious
  • Consider deleting the other async_read_some definition since it should be unused
  • Run connection throughput tests. Measure memory usage for thousands of outstanding connections.

Ref: chriskohlhoff/asio#1015

@adamncasey adamncasey force-pushed the smallbufferfix branch 2 times, most recently from af17436 to 6856be2 Compare April 1, 2022 16:55
@adamncasey
Copy link
Contributor Author

Results for connection throughput testing - as before, I'm really just trying to make sure we don't regress performance here, and comparing against the not-shown fix using a fixed buffer size. The memory usage is fairly adhoc - I just watched top while the tests ran.

Values in connections completed per second over the test run:

Test # TLS TLS TLS
Code Current Fixed Buffer 32K Tiny Buffer +ask
1 928 876 963
2 896 923 963
3 909 860 887
memory 1.3% 2.0% 1.1%

The machine used for testing had 16GB of ram - so assuming top isn't doing something crazy 1.1% is about 180MiB and 2% is about 320MiB.

$ target/release/amqpprox_perf_tester --address 'amqp://127.0.0.1:16001' --listen-address '0.0.0.0:5671' --message-size 1 --num-messages 1 --listen-cert ../../build/Linux/cert.pem --listen-key ../../build/Linux/key.pem --clients 10000 --max-threads 150

Each test run was only about 10 seconds. I hit ephemeral port issues on this host running longer tests - probably can work around this if we want to, but these numbers seemed like a reasonable comparison to me.

@@ -682,6 +682,9 @@ void Session::readData(FlowType direction)
readData(direction);
}
else {
if (readAmount > 0) {
LOG_TRACE << "read_some returned data and error. Data discarded to close sockets";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can add some more info in this trace like direction?

chriskohlhoff/asio#1015

`asio::ssl` has a bug where calling async_read_some with `null_buffers`
isn't correctly handled, and more or less immediately invokes the
provided handler with no data. This causes `amqpprox` to busy-loop
whenever a TLS socket has been accepted or opened.

There were two options for how to fix this:

1) Always read with a fixed size buffer, such as 32kb. This would
   simplify the code slightly, at the expense of needing multiple reads
   to handle larger than 32kb frames in non-TLS mode, even when the full
   frame is available to `amqpprox` in one go.
2) Ask asio::ssl to read with a very small buffer, then ask the openssl
   library how many bytes are available. This technique aligns with how
   `amqpprox`'s read loop works today. That is what is implemented here.

In theory something similar could be upstreamed into `asio::ssl`. It's a
little tricky though and this exact code couldn't handle the generic
`MutableBufferSequence` interface - we can take some shortcuts in our
code.

I've done some benchmarking to check this change isn't going to regress
performance noticeably. Data throughput tests indicate that this fix improves
performance for TLS connections over the existing code. Still running
connection throughput tests.
@adamncasey adamncasey merged commit 688cc88 into bloomberg:main Apr 12, 2022
@adamncasey adamncasey deleted the smallbufferfix branch April 12, 2022 11:39
adamncasey added a commit to adamncasey/amqpprox that referenced this pull request Oct 18, 2022
This commit changes MaybeSecureSocketAdaptor to template a few critical
types which allows us to write unit tests against some of it's behaviours

In theory this could probably also replace the SocketIntercept things, which
were a nice solution when the implementation of this class passed straight through
to the asio types.

Since working around some asio bugs bloomberg#69
and adding support for data rate limits bloomberg#88
the MaybeSecureSocketAdaptor class has expanded enough that we should
be writing unit tests for it - and probably should have done so before now.
adamncasey added a commit to adamncasey/amqpprox that referenced this pull request Oct 18, 2022
This commit changes MaybeSecureSocketAdaptor to template a few critical
types which allows us to write unit tests against some of it's behaviours

In theory this could probably also replace the SocketIntercept things, which
were a nice solution when the implementation of this class passed straight through
to the asio types.

Since working around some asio bugs bloomberg#69
and adding support for data rate limits bloomberg#88
the MaybeSecureSocketAdaptor class has expanded enough that we should
be writing unit tests for it - and probably should have done so before now.
adamncasey added a commit to adamncasey/amqpprox that referenced this pull request Oct 18, 2022
This commit changes MaybeSecureSocketAdaptor to template a few critical
types which allows us to write unit tests against some of it's behaviours

In theory this could probably also replace the SocketIntercept things, which
were a nice solution when the implementation of this class passed straight through
to the asio types.

Since working around some asio bugs bloomberg#69
and adding support for data rate limits bloomberg#88
the MaybeSecureSocketAdaptor class has expanded enough that we should
be writing unit tests for it - and probably should have done so before now.
willhoy pushed a commit that referenced this pull request Oct 19, 2022
This commit changes MaybeSecureSocketAdaptor to template a few critical
types which allows us to write unit tests against some of it's behaviours

In theory this could probably also replace the SocketIntercept things, which
were a nice solution when the implementation of this class passed straight through
to the asio types.

Since working around some asio bugs #69
and adding support for data rate limits #88
the MaybeSecureSocketAdaptor class has expanded enough that we should
be writing unit tests for it - and probably should have done so before now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants