-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
fix: handle SSL errors in client handler #551
Conversation
|
||
assert {:stop, {:shutdown, :ssl_error}} = | ||
ClientHandler.handle_event(:info, error, nil, data) | ||
end |
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.
I am not sure if it is the proper way to test it. I do not even think that it is path that should be tested at all.
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 client handler has many cases, and with this test, we can ensure that the function returns the proper response to incoming messages.
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.
I still think it is improper way to handle it. I will probably merge it, but I think that this test should be scrapped in favour of doing real TLS connection that is killed before finishing handshake or killing the connection without proper exit messages.
|
||
assert {:stop, {:shutdown, :ssl_error}} = | ||
ClientHandler.handle_event(:info, error, nil, data) | ||
end |
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.
I still think it is improper way to handle it. I will probably merge it, but I think that this test should be scrapped in favour of doing real TLS connection that is killed before finishing handshake or killing the connection without proper exit messages.
@hauleth since this was pushed live 8 hours ago in |
@blechatellier it got reverted to previous behaviour, but that particular region got reverted to previous release in general, so that issue should be gone. We are going to investigate the issue, because from what we have seen so far, it happens only with Node's |
Thanks @hauleth - happy to give you as much information as needed to help debug. |
No description provided.