Conversation
…it does not mean an error, but sometimes it does
|
Omg, this is so awesome. lgtm |
|
I hope, you say "awesome" not about my English :) |
|
Nice work! |
src/node_crypto.h
Outdated
There was a problem hiding this comment.
I really do not like default arguments, it complicates code and doesn't fit C++ style used by node...
Can I ask you to use enum for this?
There was a problem hiding this comment.
Define special enum with two values to use it in only one sitaution... Looks redundand for me.
May be just bool, without default argument?
There was a problem hiding this comment.
I second @indutny's suggestion. An enum at the call site is much more descriptive for readers of the code than a boolean is.
There was a problem hiding this comment.
Indeed, that's why I asked you :)
|
Also, can you please sign CLA ? |
|
Check new commit. I removed bidirectional shutdown as you ask. But top-level code can't help with it -- it will never know that something goes wrong. And I made HandleSSLError calls more clear. I am not competent in your code style, so may be it is still not good enought. It will be ok for me, if you will reject my pull request and fix this bug yourself. I am vain a little, but I'll stand :) I signed CLA. |
|
LGTM. @indutny? |
|
Yes, LGTM! :) Thanks. Can someone please open a bug about "not-graceful" ssl shutdown? I'm too lame to do it :) |
|
Thanks Sergey, landed in 019ad34. |
4 participants
Fixed problem:
OpenSSL contains errors in thread-specific errors stacks. Becouse of asynchronous nature of nodejs, all nodejs connections share the same errors stack. So, if you didn't process error (and clean stack) in one connection, you will surprisingly get it in another one.
Function SSL_get_error can not work correctly if before apropriate SSL/TLS call errors stack was not empty (see http://openssl.org/docs/ssl/SSL_get_error.html )
Your version of HandleSSLError always interpret zero as a non-error code. For SSL_accept, SSL_connect and SS_write it is not true (see http://openssl.org/docs/ssl/SSL_connect.html ). Apropriate errors were never processed and another connection got them.
And see comment about SSL_shutdown in my code.