AbstractApplicationContext.close() could be more forgiving of exceptions [SPR-7106]

[spring-projects-issues]

Dave Syer opened SPR-7106 and commented

AbstractApplicationContext.close() could be more forgiving of exceptions. Since it is often called in a finally block, technically it should be used like this:

try {
  context.refresh()
} finally {
  try {
    context.close();
  } catch (Exception e) {
    // swallow or log it
  }
}

If you don't catch and log/swallow you never get to see the exceptions thrown by context.refresh() (which are more interesting since they happen first).

This never used to happen in my code until Spring 3.0, at which point LifecycleProcessor will sometimes throw an exception (e.g. especially if context.refresh() has failed). There is a catch and log in there, but it only applies to quite a small block, so I assume that was adequate until the lifecycle procressor came along.

---

Affects: 3.0 GA

[spring-projects-issues]

Juergen Hoeller commented

Dave, what sort of exception was it that you were seeing there? We should catch that exception directly in DefaultLifecycleProcessor if possible, logging it with detailed local context...

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Thinking about it a bit further, it isn't actually well defined whether you'd even have to call context.close() after a context.refresh() failure... refresh() does clean up in case of an exception before propagating it to the caller, destroying all beans that have already been created etc, and setting to context to active=false. Which in turn means that context.close() won't do much since it only actually performs the full close procedure if the context is still active...

So it seems we'll have to research this a bit further. A concrete exception coming out of close() would be a great starting point.

Juergen

[spring-projects-issues]

Dave Syer commented

I'm having some trouble finding the error again, since it was tripped by a bug I have since fixed. From memory it was an exception thrown in response to an ApplicationEvent that the listener or multicaster judged to be illegal. Something like a lifecycle being stopped before it was started. but I can't find the code that was throwing that exception: I just remember it was somewhere in Spring. I'll dig around a bit more.

[spring-projects-issues]

Juergen Hoeller commented

I've refined LifecycleProcessor exception handling in several respects: We're properly wrapping a start() exception from a bean now (into an ApplicationContextException which includes some context in its message), and we're defensively catching any exception coming out of a LifecycleProcessor's onClose() method (which generally shouldn't happen).

Note that there is not much reason to call close() after a refresh() failure. This was always the intention and is restored now, with the context being marked as inactive after a refresh() failure, and hence close() being a no-op in that case. Leaking start() exceptions were able to escape that intended treatment in previous 3.0.x releases...

Juergen