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

Handle ControlThrowable exception in 'race' function #216

Merged

Conversation

rcardin
Copy link
Contributor

@rcardin rcardin commented Sep 16, 2024

I added the handling of a ControThrowable to the internals of the race function. The ControThrowable exceptions have suppression disabled by default, so it's not possible to accumulate suppressed exceptions.

Feel free to let me know if anything needs clarification or if you notice something needs to be improved.

Closes #213

val r = try Success(f())
catch
case NonFatal(e) => Failure(e)
case e: ControlThrowable => Failure(e)
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually wondering if we shouldn't introduce a third state here. So race (as opposed to raceResult) returns the first success, or returns a failure if all computations fail. So the question is, should we also wait for other computations to fail, if one of the reports throws a ControlThrowable? Maybe you can share a use-case?

I was thinking about the other exceptions that might get thrown. InterruptedException, VirtualMachineError, ThreadDeath, LinkageError are all exceptions which should cause race to finish immediately (without checking if other branches throw exceptions, or not). There's still a bug in the current implementation as to how these are handled, but I'm not 100% sure how to handle thee fatal exceptions in terms of the contract of race.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you can share a use-case?
Sure. raise4s uses a ControlThrowable to forward typed errors to the caller, as they were exceptions. So, take the following code for example:

val result: Int raises String = ox.race(
  {
    ox.sleep(500.milliseconds)
    Raise.raise("Something went wrong")
  }, {
    ox.sleep(1.second)
    1
  }
)
Raise.run(result) match {
  case value: Int => println(s"Value: $value")
  case error: String => println(s"Error: $error")
}

I expected the above code to print Value: 1 to the console, waiting for the long task to finish, since the first went in a logic error. I don't know if we should make this behavior universal for ControlThrowables.

but I'm not 100% sure how to handle the fatal exceptions in terms of the contract of race

AFAIK the Fatal exception should not be handled. I agree that if the code throws a fatal exception, the whole race must stop immediately and rethrow the fatal error.

Please let me know your thoughts and how you'd like me to proceed.

@adamw
Copy link
Member

adamw commented Sep 18, 2024

Thanks, I've extended the code to also handle other fatal exceptions

@adamw adamw merged commit 47d490f into softwaremill:master Sep 18, 2024
5 checks passed
@rcardin rcardin deleted the 213-race-function-and-controlthrowable branch September 18, 2024 12:25
@rcardin
Copy link
Contributor Author

rcardin commented Sep 18, 2024

Thanks, Adam. I saw the code you added. It sounds great. I'll keep looking to see if there is any other place in the code with the same problem we spotted here.

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.

The race function doesn't handle functions that can throw a ControlThrowable exception
2 participants