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

ExceptionHandlerMethodResolver loses all handler methods [SPR-15907] #20461

Closed
spring-issuemaster opened this issue Aug 29, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Aug 29, 2017

Kai Burjack opened SPR-15907 and commented

There is a serious bug in Spring Web MVC introduced in Spring 5 which results in the ExceptionHandlerMethodResolver (the one responsible for resolving @ControllerAdvice/@RestControllerAdvice exception handler methods) to lose all exception handler method associations under heavy memory/GC pressure.

We are currently using Spring 5 RC3 in an application and after about one day of runtime, our exception handler methods in a @ControllerAdvice class just won't get called anymore. They are being called perfectly once the application was started so there is no general issue with the code setup there.

We've investigated this issue very thoroughly and the cause is the use of an ConcurrentReferenceHashMap in (see: https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/method/annotation/ExceptionHandlerMethodResolver.java#L53)

This changed as of Version 5 in Spring from a ConcurrentHashMap to this soft/weak reference map. Now, the problem is that the garbage collector will clear the soft/weak references held in this map under heavy memory load such that no exception handler method will ever get called again and the server reports HTTP status code 500 because of the exception being caught by the upper-most servlet handler.

We also realized that the ExceptionHandlerMethodResolver is being built and populated by the ExceptionHandlerExceptionResolver. HOWEVER, there it is being held in a strongly referenced ConcurrentHashMap. This will result in the ExceptionHandlerExceptionResolver NOT losing the ExceptionHandlerMethodResolver, BUT the methods in the soft/weak map inside the ExceptionHandlerMethodResolver to be cleared once GC performs a full cycle.

The issue is easy to reproduce: All that is needed is a simple Web MVC project with a @ControllerAdvice annotated class with a @ExceptionHandler(MyException.class) annotated method and a thread which keeps on allocating memory up to the point where an OutOfMemoryError would occur and then clearing the memory.

Please fix this by at least making the field ExceptionHandlerMethodResolver.mappedMethods not a soft/weak map anymore (the exceptionLookupCache field may of course be weak/soft, since it is a cache).

Thanks!


Affects: 5.0 RC3

Attachments:

Issue Links:

  • #20416 @ControllerAdvice behaving erratically in latest milestones ("is duplicated by")
  • #20276 Extend null-safety to field level

Referenced from: commits 2b44e6e

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2017

Kai Burjack commented

I will provide you with a self-contained Spring Boot application with a unit test that reproduces this issue, soon.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2017

Kai Burjack commented

Please find attached a minimal Spring Boot example with a JUnit test that exactly reproduces the bug.
In order for the bug to reproduce, I have to force a full GC cycle, which I do via repeated allocations until a OOME, which is caught (yes I know you should not do this, but it is necessary for the test setup :) ) and then the memory freed.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2017

Juergen Hoeller commented

Good catch! As far as I can tell, this was a regression accidentally caused by our #20276 efforts in 5.0 RC3. Fixed for 5.0 RC4 now, for both web and messaging handler methods.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2017

Kai Burjack commented

Thanks for the quick fix :) BUT, in the current Git master it is still the weak/soft reference map being used? How was it fixed?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2017

Kai Burjack commented

Okay, I can confirm that with spring.version = 5.0.0.BUILD-SNAPSHOT and spring boot version = 2.0.0.BUILD-SNAPSHOT the issue does not appear anymore. Thanks!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2017

Juergen Hoeller commented

Indeed, my actual commit wasn't in yet. It's using hard references in a regular HashMap now since there is no concurrent populating anyway, just concurrent access.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 27, 2017

Alex commented

I think that the related #20416 can be closed now, thanks everybody for individuating and fixing the problem, that was really, really frustrating :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.