Skip to content

Fix core when ssl is enabled#2180

Merged
wwbmmm merged 1 commit intoapache:masterfrom
warriorpaw:master
Jun 25, 2023
Merged

Fix core when ssl is enabled#2180
wwbmmm merged 1 commit intoapache:masterfrom
warriorpaw:master

Conversation

@warriorpaw
Copy link
Contributor

@warriorpaw warriorpaw commented Mar 27, 2023

OpenSSL does not guarantee that any of its objects can be used concurrently by multiple threads:
openssl/openssl#2165
openssl/openssl#20446

Related issue with this PR: #2166

And this PR is not the best solution for this core issue.
The best way is put SSL obj's read/write operate in same event loop but this changes too much of the current code structure, i'm too lazy to do it.

@wwbmmm
Copy link
Contributor

wwbmmm commented Mar 28, 2023

Does this affect performance?

@jingshi-ant
Copy link

Does this affect performance?

about 20% slower in my 100w data set test. (mainly because of ssl enable. if remove lock and enable ssl, the speed is almost the same with lock added.)

@warriorpaw
Copy link
Contributor Author

Does this affect performance?

Brpc already piped write requests by linked list and only one bthread calls ssl_write.

In data reading, brpc use epoll in edge triggered mode and an atomic variable to make sure there is only one bthread is calling ssl_read.

Adding a mutex here will introduce some competition, but it should be very slight. Further this mutex will avoided the competition of using underlay socket fd. So, the impact of this change on performance should be almost none.

@Huixxi Huixxi added the bug the code does not work as expected label Apr 10, 2023
@wwbmmm
Copy link
Contributor

wwbmmm commented Apr 17, 2023

LGTM

@warriorpaw
Copy link
Contributor Author

@jamesge @Huixxi @wwbmmm @chenBright
Please take another look if this change can be merged, thanks!

@chenBright
Copy link
Contributor

LGTM

@wwbmmm wwbmmm merged commit b4fecac into apache:master Jun 25, 2023
yanglimingcn pushed a commit to yanglimingcn/brpc that referenced this pull request Oct 31, 2023
csun5285 pushed a commit to csun5285/doris that referenced this pull request Sep 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug the code does not work as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants