Issue 1:
David Pedowitz opened SPR-4770 and commented

Introduce @HandlerInterceptor to create a more symmetrical relationship between @Controller, @ModelAttribute and @RequestParam.

This came out of some questions and brainstorming in a Spring training session in LA, 5/1/08 with Chris Beams, Tchavdar Ivanov (FIM) and myself.

In Spring 2.5 the implementation of HandlerInterceptor has not changed leaving you to either implement the interface or extend HandlerInterceptorAdapter as before. If you use @Controller and component scan you still must define a DefaultAnnotationHandlerMapping to configure a single list of HandlerInterceptors for all Controllers.

It'd be nice if you could define a class as a @HandlerInterceptor and annotate methods as @AfterCompletion, @PreHandle and @PostHandle. The default behavior could automagically intercept all @Controllers scanned and fire according to the same contract as the original HandlerInterceptor. Also interaction could be similar to @Controller with the methods allowing use of @ModelAttribute and @RequestParam.

Example provided by Chris Beams:

// @HandlerInterceptors are @Components, thus eligible for component scanning.
// If no class args are supplied, the interceptor applies globally to all
// registered @Controllers. Naturally, specifying one or more classes narrows
// its scope.

@HandlerInterceptor({AccountController.class, CustomerController.class}) public class ExceptionLoggingHandlerInterceptor {
@AfterCompletion
public void logException(Object handler, Throwable ex) {
logger.error(...);
}

// Or to be more explicit, do it @AspectJ-style, using
// attributes to bind by name

@AfterCompletion(handler="handler", exception="ex")
public void logException(Object handler, Throwable ex) {
logger.error(...);
}

// In keeping with @Controller methods, the user could
// request the request, response, model, etc.

@AfterCompletion(handler="handler", exception="ex")
public void logException(HttpServletRequest req, Object handler, Throwable ex) {
logger.error(...);
}

// Could even have handler methods return @ModelAttributes!

@PreHandle
public @ModelAttribute("currentDate") Date currentDate(/* no params needed, none requested */) {
return new Date();
}

}

Obviously there's a lot to be worked out. Some questions we have are:

How do you express the order of the interceptors, something that comes naturally in the <list> configuratin. @Order?

Is it more natural to express the Controller classes the HandlerInterceptor applies to in the @HandlerInterceptor (above) or would something like @Controller(interceptors=(@HandlerInterceptors{class, class, ...})) be more expressive? which is more flexible?

The later does express the list naturally but you would have to repeat the list in multiple controllers to achieve the same behavior.

Maybe an XML annotation hybrid? <handlerInterceptorList id="authInterceptors">...</handlerInterceptorList> and @Controller(interceptor="authInterceptors")

Could tooling help visualize how an @Controller relates to it @HandlerInterceptors (add bonus for sure!)

Issue Links:

#17959 Apply Filter or Interceptor to Controller class via annotations ("is duplicated by")

#8695 Ability to restrict HandlerInterceptors to specific controller paths

14 votes, 16 watchers

Issue 2:
Mark Davies opened SPR-8471 and commented

A less significant version of this problem has already been raised under #10033 - a performance bottleneck affecting Wicket. However, the same issue causes a serious thread deadlock in our application, occasionally preventing application startup.

The basic issue seems to be that DefaultSingletonBeanRegistry takes a global synchronization lock when creating a singleton bean. Here is the code in that class:

public Object getSingleton(String beanName, ObjectFactory singletonFactory) {
Assert.notNull(beanName, "'beanName' must not be null");
synchronized (this.singletonObjects) {
Object singletonObject = this.singletonObjects.get(beanName);
if (singletonObject == null) {
... stuff ...
singletonObject = singletonFactory.getObject();
... stuff ...
}

The synchronized block (the same this.singletonObjects reference is used by all threads entering the method) means that Spring can only create singleton beans one at a time, regardless of their type (beanName). This clearly introduces a performance penalty if an application has a large number of singleton beans to construct, e.g. at startup.

That is not the issue affecting our code, though. We see a deadlock, caused by the following two sets of behaviour:

We have Spring-managed singleton beans which perform database access in their constructor (basically loading and caching configuration sets from the database). In order to do this they obtain database connections, which are pooled, with relatively small pool sizes. If a pooled connection is not available, the calling thread blocks and waits until one becomes free. This is usually not a problem since queries are small and rapid, so pool wait times are low, and the maximum pool size is sufficient to work the databasea at full capacity anyway.

We also have non-Spring code doing database access. Such code obtains a database connection from the same pool, purely for the lifetime of running a query and processing a result set, so again very quick for almost cases in our system. But sometimes, whilst processing the result set, we need to use a Spring-managed bean, which may have singleton scope.

You now have a deadlock - thread number one is trying to get a Spring-managed singleton bean, which is waiting for a JDBC connection in its constructor; thread number two is running database code which has the JDBC connection and is waiting to create a Spring-managed singleton of a completely different type. Both thread own the resource needed by the other, so will wait forever. (OK, if the database pool size is two or higher you need two or more threads in the second state, but this has happened to us in customer environments, in production).

Obviously our application code is almost certainly less than ideal in how we use Spring, but it seems to me that we ought to be able to use Spring-managed beans which do database access in the manner I've described, without encountering unpleasant deadlocks such as this. Note that the problem is actually much more general than this particular example of database connections: if your singleton beans require any kind of global monitor, which is also used outside of the context of Spring, you have a deadlock condition.

Both the originally-reported performance problem, and this deadlock, could be solved by a simple improvement to DefaultSingletonBeanRegistry. The getSingleton() method should not synchronize on a global monitor, but instead a monitor specific to the beanName you are instantiating. This is the correct level at which to lock - what you are trying to do is prevent a bean of a certain type being created more than once, but allowing two beans of different types to construct at the same time is perfectly reasonable. Of course you have to synchronize access to the underlying collections with a global lock, but the construction of the singleton bean does not need to be protected with the same global monitor. For instance I believe this kind of thing would do it:

public Object getSingleton(String beanName, ObjectFactory singletonFactory) {
Object bean;
Object perBeanMonitor;
synchronized (GLOBAL_MONITOR) {
perBeanMonitor = getPerBeanMonitor(beanName);
}
synchronized (perBeanMonitor) {
synchronized (GLOBAL_MONITOR) {
bean = getBeanFromRegistry(beanName);
}
if (bean==null) {
Object bean = doConstructBean(beanName, singletonFactory);
synchronized (GLOBAL_MONITOR) {
addToRegistry(beanName, bean);
}
}
}
return bean;
}

It would be extremely helpful if this approach could be implemented in this core part of the Spring framework. We rely on singleton construction to be performant and thread-safe; the fact that DefaultSingletonBeanRegistry behaves as it currently does is causing us serious production problems at client sites, and, whilst we can modify our application code to work around the issue, I do see this as a fundamental flaw in Spring - would be nice to get a quick fix please.

Affects: 2.5.6

Attachments:

test-case.zip (3.09 kB)

Issue Links:

#13428 Deadlock during application context creation ("is duplicated by")

#14470 A Java Thread deadlock has occured ("is duplicated by")

#13837 Deadlock on the beanDefinitionMap and singletonObjects ("is duplicated by")

#12374 Deadlock between DefaultListableBeanFactory and DefaultSingletonBeanRegistry, perhaps due to lazily-instantiated aspect

#14452 Non-singleton beans performance issue

#14654 Deadlock in DefaultListableBeanFactory/DefaultSingletonBeanRegistry

#14985 AbstractApplicationEventMulticaster#getApplicationListeners() should have double check for 'this.retrieverCache.get(cacheKey)' within synchronized block

Referenced from: commits dd68fec, 52124fa

19 votes, 26 watchers