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

Process : Fix exception handling in acquireCollaborativeResult() #5529

Merged

Conversation

johnhaddon
Copy link
Member

When an exception was thrown from ProcessType::run(), we were catching it and rethrowing it from the code path that initiated the collaboration. But since we were rethrowing after the run_and_wait(), collaborating threads could leave their wait() before the exception was rethrown. In this case, they would throw their own vague exception about there being no result available, and this exception could be the first thrown, and therefore the first one to surface back to the caller. We now store the original exception in TypedCollaboration::result (now a variant) so that both the initiator and the collaborator throw the same exception.

Technically this is an ABI break, because it changes a member in TypedCollaboration. But for it to cause a problem, the following conditions would need to be met :

  • Someone would need to be using acquireCollaborativeResult() already. I'm not aware of anyone ever creating their own Process subclass, so the chances of someone doing that and using a new feature that was only released yesterday are vanishingly small.
  • They would also need to compile some code with the old definition, and some with the new, which again is vanishingly unlikely, as Process subclasses are usually completely hidden.

I think it's pretty clear that the lesser evil in this case is to fix the bug.

This fixes the build on MacOS.
When an exception was thrown from `ProcessType::run()`, we were catching it and rethrowing it from the code path that initiated the collaboration. But since we were rethrowing after the `run_and_wait()`, collaborating threads could leave their `wait()` before the exception was rethrown. In this case, they would throw their own vague exception about there being no result available, and this exception could be the first thrown, and therefore the first one to surface back to the caller. We now store the original exception in `TypedCollaboration::result` (now a variant) so that both the initiator and the collaborator throw the same exception.

_Technically_ this is an ABI break, because it changes a member in TypedCollaboration. But for it to cause a problem, the following conditions would need to be met :
- Someone would need to be using `acquireCollaborativeResult()` already. I'm not aware of anyone ever creating their own Process subclass, so the chances of someone doing that _and_ using a new feature that was only released yesterday are vanishingly small.
- They would also need to compile some code with the old definition, and some with the new, which again is vanishingly unlikely, as Process subclasses are usually completely hidden.

I think it's pretty clear that the lesser evil in this case is to fix the bug.
@johnhaddon johnhaddon force-pushed the exceptionHandlingFix branch from 1546a85 to 6a02316 Compare November 4, 2023 09:20
@danieldresser-ie
Copy link
Contributor

LGTM. And I agree on not being worried about ABI in this case.

@johnhaddon johnhaddon merged commit a0c28e1 into GafferHQ:1.3_maintenance Nov 7, 2023
4 checks passed
@johnhaddon johnhaddon deleted the exceptionHandlingFix branch November 8, 2023 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants