If a task returns an exception value, do not raise it in #wait.

[Math2]

Types of Changes

• Bug fix.

Contribution

• I added tests for my changes.
• I tested my changes locally.
• I agree to the Developer's Certificate of Origin 1.1.

Notes

If a task returns an exception value (without raising it), #wait raises it.

require 'async'
Sync do
	t = Async do
		StandardError.new
	end
	t.wait # raises StandardError!
end

Seems like something that would rarely happen, but it happened to me with an assert_raises at the end of a task (apparently, it returns the exception that was raised).

[ioquatix]

This looks okay to me, although it breaks the behaviour/compatibilty.

Can you follow the existing explicit code style if ... with explicit return.

We need to be convinced that:

1. Every code path that causes to enter failed state also causes @result to be an exception (otherwise I suppose raise will fail).
2. Can there be cases where someone is assigning an error without entering failed state, expecting it to raise on #wait?

[Math2]

This looks okay to me, although it breaks the behaviour/compatibilty.

Can you follow the existing explicit code style if ... with explicit return.

Alright.

We need to be convinced that:

1. Every code path that causes to enter failed state also causes `@result` to be an exception (otherwise I suppose `raise` will fail).

Thankfully the code makes it pretty clear. @status is always set when @result is. @status is only set to :failed in one place.

2. Can there be cases where someone is assigning an error without entering failed state, expecting it to raise on `#wait`?

Hopefully there aren't any callers that expect that just returning an exception (or setting it manually with #result=) would automatically raise it, but, there's a risk I guess...

To be really safe there could be a deprecation warning instead.

There are callers that expect the exception to be available via #result after a task failed in the test suite though (and those still work fine).

[ioquatix]

I think the expectation that

Async{StandardError.new}.wait

should raise an exception is undefined at best. The fact that existing tests don't break is a testament to that.

However, with this PR, can we please add tests to cover the different use cases. In other words, let's not leave it up to chance going forward. Maybe documentation is a good idea too. Thanks!

[ioquatix]

LGTM. We might consider backporting this... but honestly, I'm not sure if it matters.

[Math2]

We might consider backporting this... but honestly, I'm not sure if it matters.

Nah, I don't think so either.

[ioquatix]

This also made me think about calling #wait on stopped tasks. Should it raise Async::Stop?

[Math2]

That could propagate and stop the calling task (if not caught) right? Should be some other exception instead.

But, calling #wait on a stopped task (expecting to get nil) is the kind of thing I would expect callers would do in practice. It could be pretty convenient.

[ioquatix]

I'll have to think about it. Such a change is tricky.