-
Notifications
You must be signed in to change notification settings - Fork 58
fix(network): use multi io_services in asio #1016
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.
Have you reproduced the bug? And how do you ensure the new code will fix it?
I did a grayscale test in the production environment, the old version machine still had this problem, but the new version didn't have. The reason of the coredump is the race condition, but in the new design there won't be a multi-thread environment in one socket |
If this is a bugfix PR, explicit use |
src/runtime/rpc/asio_net_provider.h
Outdated
|
||
private: | ||
friend class asio_rpc_session; | ||
friend class asio_network_provider_test; | ||
|
||
std::shared_ptr<boost::asio::ip::tcp::acceptor> _acceptor; | ||
boost::asio::io_service _io_service; | ||
int _next_io_service = 0; |
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 number must be atomic since multiple threads may concurrently modify it. I would recommend randomly choosing the io service such that we can totally get rid of concurrent conflict.
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.
- the operation of int is naturally atomic
- random function costs lots of time
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 statement really disappoint me, but the overall idea to be efficient maybe work. I won't give it+1. Others might do.
https://stackoverflow.com/questions/54188/are-c-reads-and-writes-of-an-int-atomic
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.
Yes, you are right. And I'm sorry for my irresponsible statement.
I did an experiment in Godbolt before, And I found i++
is one line in assembly language:
add eax, 1
. So I think this operation is thread-safety.
But optimizations for modern processors may make it more complex. And I learned a lot from this answer
I think modifying int
in multi-threads is safe(won't core dump), but it can't keep its coherence between multi processors.
Anyway, it's ub behavior to use non-atomic variables in multi-threads in C++, and I have changed my codes. You can continue reviewing it now.
++_next_io_service; | ||
if (_next_io_service >= FLAGS_io_service_worker_count) { | ||
_next_io_service = 0; | ||
} | ||
|
||
int tmp = _next_io_service; | ||
if (tmp >= FLAGS_io_service_worker_count) { | ||
tmp = 0; | ||
} |
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.
How about ensure FLAGS_io_service_worker_count is 2^N, io_service_worker_mask = FLAGS_io_service_worker_count - 1, then the code can be simplfied as:
uint32_t idx = _next_io_service.fetch_add(1);
return *_io_services[idx & io_service_worker_mask];
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.
Good idea, I have thought about it too. But it's strict to limit FLAGS_io_service_worker_count
. I'll do a speed test later to check if it's faster than two addition operations.
I did a benchmark in multi-threads env again, and found random is the quickest way, so I decide to adopt this way.
https://gist.github.com/Smityz/00426f49544348676d4ddd8b0b0eb253 |
Background
related issue: apache/incubator-pegasus#307
This issue reported a bug that the core dump will happen in some scenes. It's caused by the race condition when multi-threads read/write/close the same socket.
how to fix this bug
Original way
In the past, we used multi-threads to execute polling or callback in one event loop. But like the coredump information showed, the
Use-After-Free
may happen in the high-traffic scene. It's hard for us to add mutex to prevent this problem, so I change the way we use ASIO in this PR.New way
I use one loop per thread model in the network service, the operations of one socket are executed in the single thread. So we won't worry about race conditions anymore.
Benchmark
Original benchmark
After change
The change has no significant performance impact
Actual performance
This change has fixed the bug in our production environment.