Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Re-throw boost::coroutines::detail::forced_unwind #7351

Closed
Al2Klimov opened this issue Jul 23, 2019 · 5 comments
Closed

Re-throw boost::coroutines::detail::forced_unwind #7351

Al2Klimov opened this issue Jul 23, 2019 · 5 comments
Assignees
@dnsmichi
Labels
blocker Blocks a release or needs immediate attention bug Something isn't working core/quality Improve code, libraries, algorithms, inline docs
Milestone
2.11.0

Comments

@Al2Klimov
Copy link
Member

At the moment some of our coroutines catch exceptions during async ops, but none of them re-throw boost::coroutines::detail::forced_unwind. They have to re-throw that exception unconditionally.

There aren't any visible problems yet as we cheat our static destructors, but IMO we should not rely on this.
@Al2Klimov Al2Klimov added bug Something isn't working core/quality Improve code, libraries, algorithms, inline docs labels Jul 23, 2019
@Al2Klimov Al2Klimov changed the title Catch boost::coroutines::detail::forced_unwind Re-throw boost::coroutines::detail::forced_unwind Jul 23, 2019
@dnsmichi dnsmichi self-assigned this Jul 23, 2019
@dnsmichi dnsmichi added this to the 2.11.0 milestone Sep 9, 2019
@dnsmichi dnsmichi added the blocker Blocks a release or needs immediate attention label Sep 9, 2019
@dnsmichi
Copy link
Contributor

dnsmichi commented Sep 9, 2019

Actually this causes problems, especially on Windows with #7431. There's two things to fix:

  • For Boost Asio operations, we may just change catch (...) and remove swallowing these exceptions with the static error_code dummy.
  • For coroutine creation within asio::spawn, we must use a wrapper which ensures to catch and re-throw the exceptions accordingly. This wrapper solves the headache with any other exception being thrown from inside a coroutine.

See boostorg/coroutine#39 for details.

@dnsmichi
Copy link
Contributor

dnsmichi commented Sep 9, 2019

For the HttpServerConnection class this is already done with #7477.

dnsmichi pushed a commit that referenced this issue Sep 9, 2019
JsonRpcConnection: Don't swallow exceptions in Boost.Coroutine
ebb0826 
refs #7351
@dnsmichi dnsmichi mentioned this issue Sep 9, 2019
Merged
@dnsmichi
Copy link
Contributor

dnsmichi commented Sep 9, 2019

The wrapper is inspired by https://github.com/niekbouman/commelec-api/blob/master/commelec-api/coroutine-exception.hpp#L35

@dnsmichi
Copy link
Contributor

dnsmichi commented Sep 9, 2019

Defer() calls are also a TODO.

dnsmichi pushed a commit that referenced this issue Sep 9, 2019
Avoid the Defer-Disconnect destructor pattern with Boost.Coroutines
17d4d17 
Exceptions in Disconnect() might be thrown (this has been reworked
into error_code locally) which are swallowed inside the Destructor
for being dangerous. On the other hand, swallowing them may
corrupt the stack unwinding operation from the coroutine layer.

The best is to avoid Defer inside lib/remote and call Disconnect()
directly after breaking from other operations.

refs #7351
refs #7431
This was referenced Sep 9, 2019
Merged
Merged
dnsmichi pushed a commit that referenced this issue Sep 9, 2019
Rewrite error handling in HttpServerConnection#EnsureValidHeaders()
c5aa978 
Throwing local exceptions unnecessarily pollutes the exception
stack with immediate unwinding. Avoid this pattern at all cost within
Boost.Coroutines. MSVC may handle exceptions differently and cause
problems with stack unwinding.

refs #7431
refs #7351
This was referenced Sep 9, 2019
Merged
Closed
@dnsmichi
Copy link
Contributor

All things are implemented and the stack unwind works in both HTTP and JSON-RPC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dnsmichi dnsmichi

Labels
blocker Blocks a release or needs immediate attention bug Something isn't working core/quality Improve code, libraries, algorithms, inline docs
Projects
None yet
Milestone
2.11.0
Development

No branches or pull requests
2 participants
@dnsmichi @Al2Klimov