Catch exceptions from tasks #7072

phosit · 2024-09-25T20:55:08+02:00

phosit commented

2024-09-25 20:55:08 +02:00

It's now possible te get an exception from a function in a task.
The interface is like std::future: if you call .Get() you will get the result (as before) or the exception will be thrown.

I don't know if this also does handle windows structured exceptions. See: https://learn.microsoft.com/en-us/cpp/cpp/transporting-exceptions-between-threads
Ref: #5874

Continuation of https://code.wildfiregames.com/D4956

It's now possible te get an exception from a function in a task. The interface is like std::future: if you call .Get() you will get the result (as before) or the exception will be thrown. I don't know if this also does handle windows structured exceptions. See: https://learn.microsoft.com/en-us/cpp/cpp/transporting-exceptions-between-threads Ref: #5874 Continuation of https://code.wildfiregames.com/D4956

phosit added 1 commit 2024-09-25 20:55:11 +02:00

Catch exceptions from tasks

checkrefs / checkrefs (pull_request) Successful in 1m3s

Details

pre-commit / build (pull_request) Successful in 1m26s

Details

0ad-freebsd/pipeline/pr-main This commit is unstable

Details

0ad-windows/pipeline/pr-main This commit looks good

Details

0ad-linux/pipeline/pr-main This commit looks good

Details

0ad-macos/pipeline/pr-main This commit looks good

Details

95e2c6e4a6

It's now possible te get an exception from a function in a task.
The interface is like std::future: if you call .Get() you will get the
result (as before) or the exception will be thrown.

phosit requested review from vladislavbelov 2024-09-25 20:55:11 +02:00

phosit requested review from wraitii 2024-09-25 20:55:11 +02:00

phosit requested review from Itms 2024-09-25 20:55:11 +02:00

vladislavbelov reviewed 2024-10-02 20:21:52 +02:00

source/ps/Future.h Outdated

						
				@ -107,2 +105,3 @@

						// The caller must ensure that this is only called if we have a result.

						ENSURE(this->has_value());

						if constexpr (!std::is_void_v<ResultType>)

							ENSURE(std::get<0>(m_Outcome).has_value() || std::get<1>(m_Outcome));

vladislavbelov commented

2024-10-02 20:20:16 +02:00

Isn't explicit typing more clear and safe?

ENSURE(std::get<ResultHolder<ResultType>>(m_Outcome).has_value() || std::get<std::exception_ptr>(m_Outcome));

Isn't explicit typing more clear and safe? ```cpp ENSURE(std::get<ResultHolder<ResultType>>(m_Outcome).has_value() || std::get<std::exception_ptr>(m_Outcome)); ```

phosit commented

2024-10-03 18:28:44 +02:00

Normally i use std::tuple only with indexes. But in this case it's cleaner with types.
The benefit will be more apperent when we switch to std::variant.

Normally i use `std::tuple` only with indexes. But in this case it's cleaner with types. The benefit will be more apperent when we switch to `std::variant`.

vladislavbelov commented

2024-10-03 22:50:12 +02:00

Why do we need to switch to std::variant here?

Why do we need to switch to `std::variant` here?

phosit commented

2024-10-05 19:11:05 +02:00

A task can't result with a value and an exception.
An std::variant would clarify that.

A task can't result with a value and an exception. An `std::variant` would clarify that.

👍 1

source/ps/tests/test_Future.h Outdated

						
				@ -133,0 +139,4 @@

							using std::exception::exception;

						};

						{

vladislavbelov commented

2024-10-02 20:18:10 +02:00

You might split for different test_exception_* cases for a more clear report.

phosit marked this conversation as resolved

source/simulation2/components/CCmpPathfinder.cpp Outdated

						
				@ -802,2 +800,3 @@

					// We're done, get the exceptions from the futures.

					for (Future<void>& future : m_Futures)

						future.CancelOrWait();

						future.Get();

vladislavbelov commented

2024-10-02 20:17:33 +02:00

Code seems a bit strange, why do we call Get if we don't use the result? The CancelOrWait version seems more clear.

phosit commented

2024-10-03 18:40:11 +02:00

Like the comment explains it's done to get the exceptions from the tasks.
The interface is like std::future.

Like the comment explains it's done to get the exceptions from the tasks. The interface is like `std::future`.

vladislavbelov commented

2024-10-03 22:49:39 +02:00

Yeah, I read the comment but the naming is confusing for me. I'd prefer to have something more explicit like:

// We're done, wait till the end of the execution and get the exceptions from the futures.
for (Future<void>& future : m_Futures)
	future.WaitAndThrowIfNeeded();

Yeah, I read the comment but the naming is confusing for me. I'd prefer to have something more explicit like: ```cpp // We're done, wait till the end of the execution and get the exceptions from the futures. for (Future<void>& future : m_Futures) future.WaitAndThrowIfNeeded(); ```

phosit commented

2024-10-05 19:30:17 +02:00

Meh... I'd go with the semantics of std::future.
A Future<T>::Get call does wait for the task to finish, if the task ended in an exception rethrow that and in the other case return what the task returned.
A Future<void>::Get call does everything of the above.

The caller code is more consistent: A simple guideline could be "call Future<T>::Get on each Future". Where in your proposal it "call Future<T>::Get on each Future except if it's a Future<void> then you have to call Future<void>::WaitAndThrowIfNeeded."
The library is more consistent: There is less SFINAE.

Peoply might be confused that they think they have to call Future<T>::WaitAndThrowIfNeeded and Future<void>::Get.

Meh... I'd go with the semantics of `std::future`. A `Future<T>::Get` call does wait for the task to finish, if the task ended in an exception rethrow that and in the other case return what the task returned. A `Future<void>::Get` call does everything of the above. The caller code is more consistent: A simple guideline could be "call `Future<T>::Get` on each Future". Where in your proposal it "call `Future<T>::Get` on each Future except if it's a `Future<void>` then you have to call `Future<void>::WaitAndThrowIfNeeded`." The library is more consistent: There is less SFINAE. Peoply might be confused that they think they have to call `Future<T>::WaitAndThrowIfNeeded` and `Future<void>::Get`.