Skip to content

Handling SecurityException Security manager does not allow for static UncaughtExceptionHandler modification #42

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

Closed

Conversation

diegoocampoh
Copy link

Hi, I am Diego Ocampo from Atlassian.

Statsig server init. setting Thread (static) DefaultUncaughtExceptionHandler is causing issues. The issues come from the fact that an uncaught exception that is completely unrelated to Statsig can end up shutting down Statsig background jobs.

Also, the MainThreadExceptionHandler is re-throwing the exception, which shouldnt be done as documented in:

https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#setDefaultUncaughtExceptionHandler-java.lang.Thread.UncaughtExceptionHandler-

Note that the default uncaught exception handler should not usually defer to the thread's ThreadGroup object, as that could cause infinite recursion.

By re-throwing, the exception is then handled by ThreadGroup, which ends up calling Thread.UncaughtExceptionHandler in an infinite loop, eventually killing the thread.

In this PR I am adding a graceful exception handling for security permissions so we can initialise the application and restrict Statsig from setting this top level handler, which is problematic.

#41

…odification of static Thread.defaultUncaughtExceptionHandler
@scottwarren
Copy link

@weihao-statsig @xinlili-statsig are either of you able to help out getting this reviewed?

@weihao-statsig
Copy link
Contributor

weihao-statsig commented May 27, 2025

@scottwarren @diegoocampoh
Change looks good to me, but I saw you have another PR(#44) which is basically removing how we set up handler?

@diegoocampoh
Copy link
Author

Thanks for looking into this @weihao-statsig. We'd rather have you approve #44 since this PR catching the security manager exception is not really future proof.

Security manager will go away on Java 24 meaning this is not the right solution.

Can you have a look at #44 ? Happy to close this PR if you approve that one.

@weihao-statsig
Copy link
Contributor

weihao-statsig commented May 27, 2025

Thanks for looking into this @weihao-statsig. We'd rather have you approve #44 since this PR catching the security manager exception is not really future proof.

Security manager will go away on Java 24 meaning this is not the right solution.

Can you have a look at #44 ? Happy to close this PR if you approve that one.

I am going to do the same thing as #44 in private repo and we will have a release tonight

@diegoocampoh
Copy link
Author

Thanks @weihao-statsig !

@weihao-statsig
Copy link
Contributor

#44 is included in V2.0.8

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