-
Notifications
You must be signed in to change notification settings - Fork 62
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
Change to shutdown the connection directly in case the server do not participate close. #212
Conversation
Signed-off-by: Jianfei Hu <[email protected]>
Signed-off-by: Jianfei Hu <[email protected]>
Signed-off-by: Jianfei Hu <[email protected]>
@dio in case @Shikugawa is not avaialable. |
// directly. We choose not to use `ssl_stream::async_shutdown` since in some | ||
// case, the HTTPS server does not participate with closing | ||
// stream/connection. That would make the async_shutdown waiting forever. | ||
beast::get_lowest_layer(stream).close(); |
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 got the cause of this change. This problem is rooted from asio's implementation and I think it is better to use timeout to wait ack of close_notify
message. (But async_shutdown
doesn't have this feature. ref chriskohlhoff/asio#650)
My proposal is to note that this fix is like a workaround here and may be fixed in future work.
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 like the timeout idea.
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.
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 think that eeb5776 won't work because the timer callback won't be executed before async_shutdown
will have done ;) This is why we should wait the fix of boost/asio internal. Thanks.
Signed-off-by: Jianfei Hu <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: incfly, Shikugawa The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #211
After this PR, the issue is resolved.