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

ArcContainerImpl - catch errors during delivery of context events #2975

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Jun 26, 2019

  • @initialized(ApplicationScoped.class),
    @DESTROYED(ApplicationScoped.class) and
    @BeforeDestroyed(ApplicationScoped.class)

@mkouba mkouba added this to the 0.18.0 milestone Jun 26, 2019
@manovotn
Copy link
Contributor

@mkouba I am probably missing some background but why is it considered 'OK' not to be able to deliver these events? Especially compared to hard fail when the same thing happens for, say, request scope.

And if I read it correctly, then it may happen that you execute several observers, get an exception, dump the rest of them and proceed with application. Given that the order of those OM is not strictly defined, this can lead to weird behaviour in user application as a different subset of OMs can be executed every time before bumping into an exception. Or am I missing something?

@mkouba
Copy link
Contributor Author

mkouba commented Jun 26, 2019

Hm, well the main reason was to fix the behavior when shutting down the container where a failed notification should not affect a proper cleanup. For app init - a notification failure will abort processing of the event as usual but we would like to make sure the CDI.setCDIProvider() is invoked.

@manovotn
Copy link
Contributor

manovotn commented Jun 26, 2019

Summing up here what I told Martin over Zulip...

I think it would be more sensible to only catch the errors during shutdown, which means [Before]Destroyed events, not the Initialized one as that might leave user app in weird/undefined state.

ArcContainerImpl also calls terminate on RequestContext which invokes similar events for request context. Those should be changed similarly.

@mkouba mkouba force-pushed the arc-shutdown-catch-errors branch from 361d01a to b228c09 Compare June 26, 2019 14:00
@mkouba
Copy link
Contributor Author

mkouba commented Jun 26, 2019

@manovotn Your comments should be addressed now ;-).

- @initialized(ApplicationScoped.class),
@DESTROYED(ApplicationScoped.class) and
@BeforeDestroyed(ApplicationScoped.class)
@mkouba mkouba force-pushed the arc-shutdown-catch-errors branch from b228c09 to 916d74d Compare June 26, 2019 14:19
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Since request context can (or even has to) be activated repeatedly during the application run, I don't like much that this will cover up fails even in those cases as opposed to only during Arc shutdown. But I suppose that's alright for now...

@mkouba
Copy link
Contributor Author

mkouba commented Jun 26, 2019

Since request context can (or even has to) be activated repeatedly during the application run, I don't like much that this will cover up fails even in those cases as opposed to only during Arc shutdown.

Note that users can't handle the exception thrown from an observer method during [Before]Destroyed events anyway. So in my opinion it's ok to catch the exception and log a warning instead of rethrowing the exception.

@gsmet gsmet merged commit 2e0608f into quarkusio:master Jun 27, 2019
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.

3 participants