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

[SHIRO-668] Catch unexpected errors which can lead to oom #71

Closed
wants to merge 1 commit into from

Conversation

cpetzka
Copy link

@cpetzka cpetzka commented Aug 28, 2017

Unexpected errors in the "run" method of the validation ExecutorServiceSessionValidationScheduler can kill the validation thread an it will not be executed anymore (see StackOverflow.) This can lead to OOM's through too much sessions and can be a security risk by never ending sessions.
Catching "Throwable" is not the cleanest solution but it garant that the thread will be executed in the future.

@cpetzka cpetzka closed this Aug 28, 2017
@cpetzka cpetzka deleted the potential_oom branch August 28, 2017 19:53
@cpetzka cpetzka restored the potential_oom branch August 28, 2017 19:53
@cpetzka cpetzka reopened this Aug 28, 2017
@fpapon fpapon self-requested a review February 19, 2019 19:01
@fpapon
Copy link
Member

fpapon commented Feb 19, 2019

Retest this please

@asfgit
Copy link

asfgit commented Feb 19, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Shiro-pr/47/

@fpapon
Copy link
Member

fpapon commented Feb 19, 2019

retest this please

@asfgit
Copy link

asfgit commented Feb 19, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Shiro-pr/48/

@fpapon fpapon changed the title Catch unexpected errors which can lead to oom [SHIRO-668] Catch unexpected errors which can lead to oom Feb 21, 2019
@fpapon
Copy link
Member

fpapon commented Feb 21, 2019

@bdemers have you any objection for this PR?

@bdemers
Copy link
Member

bdemers commented Feb 21, 2019

We might want to take another look at this one, catching Throwable might not be the best option.
@cpetzka if you are still following this (and no excuse for the delay) what type of exception were you seeing?

@cpetzka
Copy link
Author

cpetzka commented Feb 21, 2019

Sorry, I can't tell you what exception it was. I don't think it was logged. There were entries in the log every 5 minutes that the validation thread was running normally. Then the entries stopped and old sessions were no longer removed. So I didn't fix the reason for the expedition but built this workaround.

@fpapon
Copy link
Member

fpapon commented Feb 21, 2019

May be we can use a UncaughtExceptionHandler in the Thread initialization to have more informations. If there is a problem with a particular Session restart a new thread will not fix the problem. I agree with @bdemers that it will be usefull to have more information about the actual exception and try to fix the origin.

@cpetzka
Copy link
Author

cpetzka commented Feb 25, 2019

If the problem is only a short disconnect from SessionStore/Cache, a simple reboot would be sufficient. However, if it is always the same session that triggers an error, the error would happen regularly but also be logged. With the help of log monitoring, the administrator can be informed about repeated errors.

@bdemers
Copy link
Member

bdemers commented Feb 25, 2019

Thanks @cpetzka I can see that.

Maybe we should catch Runtimes, and re-throw everything else. That would allow temporary exceptions like connection issues to be caught and logged (and the thread to continue), while other system errors to bubble up.

@fpapon's idea for handling errors could be helpful too

@fpapon
Copy link
Member

fpapon commented Dec 10, 2019

supercedes by #186

@fpapon fpapon closed this Dec 10, 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.

None yet

4 participants