-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Connection: Fix unexpected server disconnection due to OpenSSL thread safety problems #9
Connection: Fix unexpected server disconnection due to OpenSSL thread safety problems #9
Conversation
581d625
to
82b660b
Compare
39870fa
to
a357013
Compare
The failing CI should be fixed by rebasing against current master branch. @davidebeatrici what is the plan with this PR? |
I'm a bit concerned about locking in a function that may be called several times in a second... maybe we should let the decision up to the API user. |
I vote strongly against that. If OpenSSL explicitly states that one should use single-thread access, then that's exactly what we should do. |
Well, that's what we do inside the library itself. It's the example code that has to be fixed... |
Sounds good, but I dont know about my time to implement Krzmbrzl approach |
I'm wondering though: does this even have to be part of the public API? Individual connections and especially writing data to the respective sockets seems like something that should be abstracted away by the library, doesn't it? |
Connections can be added to |
Correct, but if we don't expose that to the end user, that's one thing less to worry about. Then we can manage the threading internally and keep the library users out of it. |
a357013
to
9305095
Compare
… safety problems From the OpenSSL FAQ (https://www.openssl.org/docs/faq.html): "... an SSL connection cannot be used concurrently by multiple threads. This is true for most OpenSSL objects."
9305095
to
90560da
Compare
While using the library for a new project I encountered the issue several times, thus I decided to rebase this and merge it right away. My initial comment is still valid, but I vastly overestimated the cost of acquiring and releasing a mutex. @botanegg The fix you proposed is simple and effective. @Krzmbrzl's idea would avoid the need for a mutex, but it adds quite some complexity and is actually not guaranteed to be the best mechanism in terms of performance. |
Its a dirty 15 minutes quick fix to show the idea
OpenSSL FAQ https://www.openssl.org/docs/faq.html said about only one thread should use a socket
Partially_Closes #7