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

Exceptions in role methods are silently discarded #18

Closed
martinmo opened this issue May 13, 2018 · 5 comments
Closed

Exceptions in role methods are silently discarded #18

martinmo opened this issue May 13, 2018 · 5 comments
Labels

Comments

@martinmo
Copy link
Contributor

Consider the following example (also available at https://github.com/martinmo/SCROLL/tree/scroll-problems):

object SwallowedException extends App {
  class CoreType

  class ExceptionShowcase extends Compartment {
    class Exceptional {
      def roleMethod: Unit = {
        println("Exceptional::roleMethod()")
        throw new Exception("catch me if you can")
      }
    }
  }

  new ExceptionShowcase() {
    val core = new CoreType()
    core play new Exceptional()
    (+core).roleMethod()
    throw new AssertionError("should not be reached")
  }
}

Expected output:

Exceptional::roleMethod()
java.lang.Exception: catch me if you can
	at <...>
	at scroll.examples.SwallowedException.main(SwallowedException.scala)

Actual output:

Exceptional::roleMethod()
java.lang.AssertionError: should not be reached
	at scroll.examples.SwallowedException$$anon$1.<init>(SwallowedException.scala:21)
	at scroll.examples.SwallowedException$.<init>(SwallowedException.scala:17)
	at scroll.examples.SwallowedException$.<clinit>(SwallowedException.scala)
	at scroll.examples.SwallowedException.main(SwallowedException.scala)

Another example is the CheckingsAccount role in the Banking example, which throws an IllegalArgumentException that gets also silently swallowed.

@max-leuthaeuser
Copy link
Owner

Those are wrapped into Scala's Either.
The following fix is suggested:

object SwallowedException extends App {
  class CoreType

  class ExceptionShowcase extends Compartment {
    class Exceptional {
      def roleMethod: Unit = {
        println("Exceptional::roleMethod()")
        throw new Exception("catch me if you can")
      }
    }
  }

  new ExceptionShowcase() {
    val core = new CoreType()
    core play new Exceptional()
    (+core).roleMethod() match {
      case Left(_) =>
        // correct
        println("gotcha")
      case Right(_) =>
        throw new AssertionError("should not be reached")
    }
  }
}

We should add this here.

@martinmo
Copy link
Contributor Author

This may be a solution for expected, recoverable exceptions (i.e., "checked" in Java), if there is a way to get the exception object (is there a way? glanced over the Scala docs, doesn't seem so). I also understand it's nicely functional, and so on, but:

Its definitely not a solution for the exception types that are thrown for unrecoverable programming errors, such as NullPointerException, StackOverflowError, ArithmeticError, OutOfMemoryError, etc. I cannot believe that Either discards them. This is just like this Java anti-pattern: try { doSomething(); } catch (Throwable e) {}.

So, currently, the program just will continue to run even if a unrecoverable error in a role occured. But shouldn't it just crash by default (aka, fail early)?

@max-leuthaeuser
Copy link
Owner

Yes, you are right. (Also see here)

In the current design of the API (all calls to applyDynamic and such), the earliest point to fail would be an explicit match on the resulting Either.

@max-leuthaeuser
Copy link
Owner

Throwables in role methods are no longer swallowed. Please review.

max-leuthaeuser pushed a commit that referenced this issue May 16, 2018
@martinmo
Copy link
Contributor Author

Works as expected 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants