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.

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

Convert Exceptions to Throwables #28

Merged
merged 3 commits into from Oct 19, 2021
Merged

Convert Exceptions to Throwables #28

merged 3 commits into from Oct 19, 2021

Conversation

skoppe
Copy link
Collaborator

@skoppe skoppe commented Aug 27, 2021

Throwables are required because we cannot let anything get past us. In single
threaded applications you only have to deal with Exceptions. Any error will
simply propagate upwards and terminate the application. In multi-threaded
applications you cannot have threads die willy-nilly, instead you want the owner
(ultimately the main thread) to know it errored.
and rethrow.

If we only do it for Exceptions, any triggered assert will kill the thread while
the rest of the application hangs waiting until the thread is done.

That is why we catch throwables, move them to the owner, and rethrow there.

@skoppe
Copy link
Collaborator Author

skoppe commented Aug 27, 2021

I am thinking of having the syncWait always throw when the Throwable is actually an Error. Normally it is fine if you discard the result of an async task, even if it is an exception, but if it is an Error (e.g. AssertError), you don't want to accidentally discard it.

By rethrowing it we force program termination by bubbling the Error upwards. If necessary users can still catch it, but by default we get proper behavior.

@nordlow
Copy link
Contributor

nordlow commented Aug 27, 2021

If we only do it for Exceptions, any triggered assert will kill the thread while
the rest of the application hangs waiting until the thread is done.

I'd prefer emphasizing like

If we only do it for Exceptions, any triggered assert (always throwing an AssertError never being an Exception but always a Throwable) will kill the thread while the rest of the application hangs waiting until the thread is done.

I've never reflected over that assert doesn't throw an Exception. Btw, how are assert allowed to be called in a nothrow and potentially @nogc context? Aren't AssertErrors allocated on the GC?

@skoppe
Copy link
Collaborator Author

skoppe commented Aug 27, 2021

I am thinking of having the syncWait always throw when the Throwable is actually an Error.

Nice idea except that a lot of Error's in D aren't actually threadsafe, that includes: SwitchError, AssertError, RangeError, ArraySliceError, ArrayIndexError, FinalizeError, OutOfMemoryError, InvalidMemoryOperationError, ForkError.

So hitting a range error on multiple threads corrupts memory. Yay.

EDIT: it isn't as bad, but its not nice either: #28 (comment)

@nordlow
Copy link
Contributor

nordlow commented Aug 27, 2021

I am thinking of having the syncWait always throw when the Throwable is actually an Error.

Nice idea except that a lot of Error's in D aren't actually threadsafe, that includes: SwitchError, AssertError, RangeError, ArraySliceError, ArrayIndexError, FinalizeError, OutOfMemoryError, InvalidMemoryOperationError, ForkError.

So hitting a range error on multiple threads corrupts memory. Yay.

That sounds crazy. How do you deduce that they aren't thread-safe? And what do you mean by "corrupts memory"?

source/concurrency/operations/toshared.d Outdated Show resolved Hide resolved
source/concurrency/package.d Outdated Show resolved Hide resolved
source/concurrency/stream.d Outdated Show resolved Hide resolved
@skoppe
Copy link
Collaborator Author

skoppe commented Aug 27, 2021

I am thinking of having the syncWait always throw when the Throwable is actually an Error.

Nice idea except that a lot of Error's in D aren't actually threadsafe, that includes: SwitchError, AssertError, RangeError, ArraySliceError, ArrayIndexError, FinalizeError, OutOfMemoryError, InvalidMemoryOperationError, ForkError.
So hitting a range error on multiple threads corrupts memory. Yay.

That sounds crazy. How do you deduce that they aren't thread-safe? And what do you mean by "corrupts memory"?

Looking at it again it isn't as bad as I originally thought.

Most throwing functions in core.exception throw use staticError, which constructs an Error in some static storage. I had overlooked the fact that the static storage is actually thread local.

It is still a problem though if you have successive Errors on the same thread. The later one will overwrite the previous one.

It is also a problem if you hold on to the Error until after the thread is gone (tls is destructed).

@nordlow
Copy link
Contributor

nordlow commented Aug 27, 2021

Looking at it again it isn't as bad as I originally thought.

Most throwing functions in core.exception throw use staticError, which constructs an Error in some static storage. I had overlooked the fact that the static storage is actually thread local.

It is still a problem though if you have successive Errors on the same thread. The later one will overwrite the previous one.

It is also a problem if you hold on to the Error until after the thread is gone (tls is destructed).

I see. Thanks for the clarification.

What about

private __gshared AssertHandler _assertHandler = null;

? When is that set?

@skoppe
Copy link
Collaborator Author

skoppe commented Aug 28, 2021

What about

private __gshared AssertHandler _assertHandler = null;

? When is that set?

I think it is mostly null for regular applications. Don't think many people use it.

For me it has two problems:

  1. it is only used for asserts, not range errors etc.
  2. anybody can change it.

@skoppe
Copy link
Collaborator Author

skoppe commented Aug 28, 2021

I've never reflected over that assert doesn't throw an Exception.

It throws an Error. Both Exception and Error inherit from Throwable. Exceptions you can safely catch, Errors you shouldn't.

Btw, how are assert allowed to be called in a nothrow and potentially @nogc context?

Errors thrown in core.exception are allocated on thread local storage, see staticError mentioned above. So they don't involve GC.

How can they be thrown in nothrow? Error is special, you can always throw it, even in nothrow code. Except nothing will get cleaned up because in nothrow code the compiler doesn't emit cleanup code (I suppose I doesn't call scope(exit){} either). Proof: https://run.dlang.io/is/p60EcF

That is why catching a Throwable is dangerous.

This all suggests when we get an Error, we should terminate as soon as possible.

@skoppe
Copy link
Collaborator Author

skoppe commented Aug 30, 2021

So, in the end I decided to go with a controlled shutdown. This is what druntime is doing and I don't see a good reason to stand out here.

Senders themselves and the algorithms around them won't catch Errors for you, when one happens they will just bubble up to whatever code started those senders. If that is the main thread, then druntime will catch it for you, print it, and terminate the runtime followed by exiting the program. If it happens to be a non-main thread, druntime's Thread class will catch the Error and rethrow it once your program joins the thread (which it should!).

If you use a Sender that starts a new thread (a ThreadSender or an StdTaskPool currently), those will catch any Errors on those threads, send them over to whoever is awaiting the Sender (whoever called .syncWait) where they will get rethrown. If that is nested on another thread it will be sent to whoever awaits that, where they will be rethrown.

In contrast to regular Exceptions if a syncWait receives an Error it will rethrow immediately. This is to ensure you don't discard Errors unbeknownst.

At any time users can catch Error's themselves and implement different behavior if so desired. Although it deserves to be said, if an Error gets throw in or over @nogc code, any destructors - and likely any scope(exit) or scope(failure) - is not going to be called.

return new ThrowableClone!AssertError(t.info, a.msg, a.file, a.line, a.next);
if (auto r = cast(RangeError)t)
return new ThrowableClone!RangeError(t.info, r.file, r.line, r.next);
if (auto e = cast(Error)t)
Copy link
Contributor

@nordlow nordlow Aug 30, 2021

Choose a reason for hiding this comment

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

Why specialize handling of AssertError and RangeError only when there are sub-classes of Error such as FinalizeError? Is it because those are the only ones with constructor parameters different from Error's?

Wouldn't it have been useful (and likely faster instead of successive calls to dynamic-cast) for Error (and Object in the general case) to have an abstract dup member that preserves typeinfo? Explicitly implemented for each sub-class, though. Just a thought.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why specialize handling of AssertError and RangeError only when there are sub-classes of Error such as FinalizeError?

I had a whole list. Then realized a lot of those where actually added in latest master only. Which made be realize I will never be able to specialize for all of them. So then I did the most useful ones only.

Wouldn't it have been useful (and likely faster instead of successive calls to dynamic-cast) for Error (and Object in the general case) to have an abstract dup member that preserves typeinfo?

It would be immensely useful. But there isn't one. I don't think one will be added as well.

Copy link
Contributor

@nordlow nordlow Aug 30, 2021

Choose a reason for hiding this comment

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

Ok, cool. Why don't you think such a member will be added?

Copy link
Contributor

@nordlow nordlow Aug 30, 2021

Choose a reason for hiding this comment

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

Copy link
Contributor

@nordlow nordlow Oct 12, 2021

Choose a reason for hiding this comment

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

There's also the question of dup vs deep dup. Maybe a deep dup be partially inferred using .tupleof.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok with me to merge this as is, @skoppe and maybe add an issue about handling of more sub-classes of Error.

Throwables are required because we cannot let anything get past us. In single
threaded applications you only have to deal with Exceptions. Any error will
simply propagate upwards and terminate the application. In multi-threaded
applications you cannot have threads die willy-nilly, instead you want the owner
(ultimately the main thread) to know it errored.
and rethrow.

If we only do it for Exceptions, any triggered assert will kill the thread while
the rest of the application hangs waiting until the thread is done.

That is why we catch throwables, move them to the owner, and rethrow there.
@skoppe skoppe merged commit c902de1 into master Oct 19, 2021
@skoppe skoppe deleted the throwable branch November 19, 2021 12:56
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.

None yet

2 participants