Skip to content

Improve error handling in on_accept#750

Merged
ras0219-msft merged 4 commits intomicrosoft:masterfrom
sridmad:master
Aug 3, 2018
Merged

Improve error handling in on_accept#750
ras0219-msft merged 4 commits intomicrosoft:masterfrom
sridmad:master

Conversation

@sridmad
Copy link
Copy Markdown
Member

@sridmad sridmad commented May 4, 2018

  1. Handle operation_abort error code. Indicates that the listen socket is closed

  2. Handle exceptions thrown by start. Setting ssh context can throw exceptions, catch them, close the connection and continue handling connections.

  3. Continue accepting new connections for any error_code other than operation_abort.

@sridmad
Copy link
Copy Markdown
Member Author

sridmad commented May 4, 2018

@kavyako , please review

@sridmad
Copy link
Copy Markdown
Member Author

sridmad commented May 4, 2018

Addresses #742

@garethsb
Copy link
Copy Markdown
Contributor

garethsb commented May 6, 2018

Is it possible to receive operation_aborted for a temporary error condition that shouldn't cause the listener to stop accepting? Conversely is there any other error code that indicates an unrecoverable error that should cause the listener to stop accepting?

@sridmad
Copy link
Copy Markdown
Member Author

sridmad commented May 10, 2018

@garethsb-sony, I found a different mechanism to detect connection closed in a asio sample at https://github.com/chriskohlhoff/asio/blob/master/asio/src/examples/cpp03/http/server/server.cpp. However this pattern does not handle all cases correctly. There are cases where on_accept is with operation_aborted before m_acceptor.is_open() is able to detect that the connection is closed. Most probably due to a race in the shutdown case.

My change will not regress existing behavior but at least allows the server to continue listening on other errors while having the same behavior for operation_aborted. I was not and expert on Linux and was not able to find much information about other cases that can result in operation_aborted.

Copy link
Copy Markdown
Contributor

@garethsb garethsb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's necessary to take the lock again when spinning off another async accept?

@sridmad
Copy link
Copy Markdown
Member Author

sridmad commented May 15, 2018

@garethsb-sony, without the lock, m_acceptor can be reset between the null check and using the variable.

@garethsb
Copy link
Copy Markdown
Contributor

Thanks. OK - yes, m_acceptor is reset under the mutex in hostport_listener::stop. Now I'm wondering whether hostport_listener::start should also take the lock in order to set m_acceptor.

@sridmad
Copy link
Copy Markdown
Member Author

sridmad commented May 18, 2018

@garethsb-sony, I cannot see anyone using an instance of hostport_listener before Start completes. For all practical purposes Start is the constructor for the type and having multiple threads calling Start on the same instance is a bug. Existing calls to Start on this type all ensure that Start is called from only one thread and other members are not called unless Start has completed.

@ras0219-msft ras0219-msft merged commit c8c9227 into microsoft:master Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants