-
Notifications
You must be signed in to change notification settings - Fork 286
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
WIP: WELD-2503 Create Exception handler for applications run using StartMain #1838
WIP: WELD-2503 Create Exception handler for applications run using StartMain #1838
Conversation
Can one of the admins verify this patch? |
ok to test |
Generic array types are problematic
I've updated a previous POC we created here at Penn State to use the new
After this commit, we've added a custom (and admittedly simplified)
|
retest this please |
Alright, I finally got down to this, so here are some thoughts... Thanks for PoC, it makes things a lot clearer. I can see the point of this effort and I think it's a good idea to be able to handle exception in a custom way. One thing I find a bit weird is that this approach will only work if you are using the built-in main method. So the question is, whether it's good "default" behaviour. Since it doesn't change the way it behaved up until now, I'd say it's fine. P.S. I know streams are cool and all, but what's wrong with |
|
||
Stream<Class<? extends Throwable>> getExceptionTypes(); | ||
|
||
void handle(Throwable t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would simplify the API so that ExceptionHandler
only declares boolean handle(Throwable t)
which returns true if the handler is able to handle the given exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would lean towards two methods, supports()
and handle()
, much like it's designed in Junit for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JUnit 5 ParameterResolver
was actually my inspiration for this interface ... I've simplified it a bit now but the user will be responsible for all the supports()
logic now. Any chance that weld-core can be setup to also run JUnit 5 tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance that weld-core can be setup to also run JUnit 5 tests?
That shouldn't be problem, it can be run in parallel with JUnit 4, though I am not sure what is the actual benefit?
Plus any more complex testing that pure unit testing has to stick with JUnit 4 because Arquillian doesn't support it ATM (at least last time I checked it didn't).
|
||
@Override | ||
public Stream<Class<? extends Throwable>> getExceptionTypes() { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would result in NPE (see also the default supports()
implementation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DefaultExceptionHandler
has to be at the end of the list and isn't registered via the ServiceLoader
. See the orElse
clause in StartMain
.
@@ -0,0 +1,21 @@ | |||
package org.jboss.weld.environment.se; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing license header...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also in the ExceptionHandler
interface.
new StartMain(args).go(); | ||
System.exit(0); | ||
} catch (Throwable t) { | ||
StreamSupport.stream(exceptionHandlerLoader.spliterator(), false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no need to use streams here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the use of the Stream
here but agree that it's a bit out of place in the ExceptionHandler
interface. (in this case, the findFirst()
and orElse()
methods eliminate a lot of if/elseif logic)
It doesn't change the default behavior but it does represent code that has to be maintained in the future. My opinion is that it's probably not the type of code that will cause maintenance issues as it's functionality is pretty clear. In any case, I'll leave that decision up to you.
Should the
I was looking over this code with a colleague and we both realized that the signature in I have in fact "wrapped" the |
Can't see how this would work. All I was saying is that I 'think' (I cannot know for sure) that a lot of people using Weld SE also define their own
Yea it would, but it's complexity that user themselves can introduce if they wish to. Not really our concern. What we could do is to allow using I'd either allow for one handler only, or enable some way to order your handlers. WDYT @mkouba ? |
I just removed the System.exit(0) as in WELD-2502 so I wouldn't forget |
@mkouba Is there something I need to do to move this off "hold"? |
Hey Steve, thanks for the nudge, this has gone somewhat stall. So, I talked this through with Martin and here is what we agreed on:
Let me know if it sounds reasonable and if you are fine making these changes. |
That all sounds reasonable ... I'm really busy at work this month but I can dig into this work in a few weeks. Would I've been pushing the https://github.com/eclipse/ConfigJsr and https://github.com/smallrye/smallrye-config projects to clean up how their validation errors are reported so I might yet be able to catch Thanks! |
+1, this would be my choice as well ;-) |
Ok, I have been outvoted, |
@smoyer64 Just to let you know, next release is 3.1.0 meaning this PR should get in. |
@smoyer64 a casual friendly inconspicuous poke |
ouch!
I still plan to get back to this soon but things at work have been quite
busy.
…On Mon, Nov 26, 2018 at 7:35 AM Matej Novotny ***@***.***> wrote:
@smoyer64 <https://github.com/smoyer64> a casual friendly inconspicuous
poke
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1838 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAUCjeYDop9KL5GMMHxE0EZ3PEuqNKT-ks5uy-AggaJpZM4UdRj6>
.
--
Sent from my iBerry MacPhone
|
Weld release is scheduled very early in Feb, this issue should either get a solid PR by the end of Jan, or we will have to postpone it. |
Okay ... I've got perhaps a day I can spare and I don't think it's too onerous. I think I'm good to go based on the previous discussion but I'm wondering how much documentation should be included. I'm assuming that needs to go in the users guide? |
The SE part of Weld docs might be the place I think - http://docs.jboss.org/weld/reference/latest/en-US/html_single/#weld-se |
Closing because the PR has been stale for far too long. |
Work in progress towards WELD-2503 - please review.
Theory
ExceptionHandler
's can be registered with StartMain through theServiceLoader
mechanism. EachExceptionHandler
specifies one or more types of that it will support (one danger is that only the firstExceptionHandler
found for a type will be called and theServiceLoader
has no mechanism for ordering the services it loads).An exception handler can:
Throwable
Throwable
System.exit()
-StartMain
StartMain
has been altered to return an exit code of zero if the program terminates normally or an exit code of one if the program terminates due to an exception. Of course, the application itself is still free to exit with its chosen error code at any time - theExceptionHandler
is only called when any checked or unchecked exception is allowed to reach themain()
method.It's recognized that some
Throwable
instances may contain very complex internal structures - understanding this content is solely the responsibility of anExceptionHandler
registered for the offending type.Outstanding tasks
ExceptionHandler
registration toStartMain
DeploymentException