[WIP] Add cache-able ControlThrowables [ci: last-only] #7165
Conversation
|
@viktorklang could I get your thoughts on this? |
|
Since there is no exception-caching going on here, I'd suggest a different prefix like I'm also wondering what the relationship between these classes and |
| override def fillInStackTrace(): Throwable = super[Throwable].fillInStackTrace() | ||
| } | ||
|
|
||
| class Cached(message: String) |
There was a problem hiding this comment.
also, in what sense is this "cached"? It appears to just be a Throwable with neither stack trace nor suppression.
There was a problem hiding this comment.
It is called "cached" because it is intended to be used for cached ControlThrowables, where e.g. having suppressed exceptions would lead to memory leaking. For non-cached ControlThrowables (e.g. NonLocalReturnControl), though a suppressed exception will probably get ignored anyway, it doesn't hurt to add a suppressed exception because the NonLocalReturnControl will go out of scope and it will all get GCed anyway.
There was a problem hiding this comment.
I am open to other names
There was a problem hiding this comment.
Hmm, ok. I'm not a fan of the name. Stateless might be better, or Immutable, or even Cacheable.
My brilliant idea of "what if we just override addSuppressed as a no-op, too" is stymied by Java's library designers thinking they know better...
There was a problem hiding this comment.
I'm okay with Cacheable. Immutable sounds weird, cuz exceptions aren't usually thought of as mutable. Maybe Stateless, but I wouldn't usually think of an exception as statefull either.
I threw this together in like 15 minutes at 3am, so the name wasn't intended to be final anyway xD
NthPortal
changed the title
| */ | ||
| trait ControlThrowable extends Throwable with NoStackTrace | ||
|
|
||
| object ControlThrowable { |
There was a problem hiding this comment.
Is this wrapper object needed?
There was a problem hiding this comment.
idk. I wanted it to be clear that they're all ControlThrowables
|
@edmundnoble they (like |
NthPortal
changed the title
|
@NthPortal should this stay open...? |
|
@SethTisue I don't know. I think it would be good to merge it, or at least discuss whether or not it is a viable solution to the problem, but it didn't seem to really get many eyes on it. I don't think I ever got @viktorklang (with whom I tangentially discussed this topic wrt It doesn't seem too controversial to me (though perhaps the names could use some work), though I know we're supposed to be in a feature freeze. If you think it needs more motivation, I could perhaps try to motivate it better. |
| trait ControlThrowable extends Throwable with NoStackTrace | ||
|
|
||
| object ControlThrowable { | ||
| sealed trait CacheableBase extends ControlThrowable { |
There was a problem hiding this comment.
This name doesn't really reflect what it does though.
There was a problem hiding this comment.
The base trait is mainly there to reduce code duplication, and should actually be private if possible (I don't know if the compiler will allow it)
| override def fillInStackTrace(): Throwable = this | ||
| } | ||
|
|
||
| class Cacheable(message: String) |
There was a problem hiding this comment.
This ought to be CacheableThrowable, right?
There was a problem hiding this comment.
Yes, but (a) ControlThrowable already has Throwable in its name, and (b) it's the "default", so I wanted it to have a shorter name.
|
@NthPortal Apologies, I totally dropped the ball on this one. :( |
|
It looks good. There should more doc comments why it exists and how it works internally. And the name is not obvious, it only makes sense once you know what it does.. Maybe something more blunt like |
|
@viktorklang no worries |
|
@lrytz I agree with you that I'm not a huge fan of The shed could definitely use a paint job |
|
Another possible name:
More ideas welcome. |
|
It's very tempting for me to join in the discussion about naming, but I think 2.13 is the time to bite the bullet and just do the simple thing, at the cost of very minor migration discomfort. Obviously, I think the linked PR is simple. Maybe there is a real-world use case for requiring a mix-in, but what could that be? |
|
I also wondered if trait parameters saves traitness here, but I read in the sip that no secondary ctors, so making message optional is harder. (Maybe a default arg would be OK? But less efficient. ControlThrowables should be lightweight as possible.) |
|
Closing in favour of #7413. Will reopen if that one doesn't pan out. |
9 participants
Add subclasses of
ControlThrowablewhich strictly disable stack tracewriting and exception suppression.
This has implications for Java's try-with-resources and
scala.util.Using,both of which suppress exceptions.
Fixes scala/bug#11116