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

Fixed stale database connection release #1526

Merged
merged 2 commits into from Apr 8, 2014

Conversation

Projects
None yet
4 participants
@rofreytag
Copy link

rofreytag commented Apr 3, 2014

Mailing list: https://groups.google.com/forum/#!topic/liftweb/Hn0bdboGzAU

getAutoCommit, rollback and commit can throw exceptions on stale connections, which was preventing a correct connection release. (c.releaseFunc() was never called in that case)

Regards
Robert

Robert Freytag
Fixed stale database connection release
 - getAutoCommit, rollback and commit can throw exceptions
   on stale connections, which was preventing
   a correct connection release

@fmpwizard fmpwizard added this to the 2.6-M3 milestone Apr 3, 2014

}
} catch {
case e: Exception =>
logger.debug("Swallowed exception during connection release. ", e)

This comment has been minimized.

@farmdawgnation

farmdawgnation Apr 4, 2014

Member

I'm not a fan of the catch-all exception. Is there a particular class of Exception that gets thrown when a connection is stale? Could we just catch that one?

@rofreytag

This comment has been minimized.

Copy link
Author

rofreytag commented Apr 5, 2014

That part of the code uses the Helpers.tryo() a lot, which is also a catch-all directive. Thats why I was following the already implemented pattern. In this case it would be possible to narrow the exception to java.util.SQLException. If we do that, however, the connection would not be released correctly, in the event of another Exception. As I mentioned in the mailing-list, the non-released connection will mess up the whole application.

We could also put the call to release in finally clause, so a non-SQLException will get thrown to the caller.

So I leave it to you guys to make a call on that. And I can implement either change, if requested.

@farmdawgnation

This comment has been minimized.

Copy link
Member

farmdawgnation commented Apr 7, 2014

That part of the code uses the Helpers.tryo() a lot, which is also a catch-all directive. Thats why I was following the already implemented pattern.

I think you misunderstood the pattern a bit. Or perhaps the pattern is just misused here. tryo has very well defined behavior. Specifically, it attempts to execute code, returning a Full of the result if it was successful or a Failure containing the exception if it threw an exception.

This code will eat all exceptions, only logging them at the debug level.

It may be true that tryo is being used in such a way that things aren't getting logged or bubbled up in this code block. I'd say that's an antipattern and design flaw in this code, and we probably shouldn't let any more code through that follows that pattern.

In this case it would be possible to narrow the exception to java.util.SQLException. If we do that, however, the connection would not be released correctly, in the event of another Exception. [...] We could also put the call to release in finally clause, so a non-SQLException will get thrown to the caller.

Let's do that. (Put the release in finally.) I think that's the best compromise we have right now with the way this code is structured.

Thanks!

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Apr 7, 2014

Yes, generally the correct pattern for resource management is that releases go in a finally block.

@rofreytag

This comment has been minimized.

Copy link
Author

rofreytag commented Apr 7, 2014

I think you misunderstood the pattern a bit. Or perhaps the pattern is just misused here. tryo has very well defined behavior.

Yes it is misused here as a swallow-all. The method releaseConnectionNamed is called in two cases: trough use and trough the LoanWrapper. In both cases it is executed in a finally clause, so throwing an exception in there would overwrite an already thrown exception. As I see it there are two cases:

  1. The connection was already stale/closed when it was used to do things in the user code. In this case an exception is already thrown to the user by the driver, and the exception in the release-code should be swallowed, as it would override the already created exception.
  2. The connection became stale/closed just before executing the release-code. In this case it would be ok to throw the exception up to the user. But that behavior would break clearThread, as it relies on releaseConnectionNamed to not throw.

Therefore, I narrowed the exception to SQL and introduced the finally clause as we discussed before, and changed the log-level to error, so the user is informed either way that the connection-release was not going well. As you said, that may be a good compromise, without having to change too much in this class for that particular case.

@farmdawgnation

This comment has been minimized.

Copy link
Member

farmdawgnation commented Apr 8, 2014

Rock on. Thanks for the contribution. 👍

farmdawgnation added a commit that referenced this pull request Apr 8, 2014

Merge pull request #1526 from rofreytag/db_realeasecon_fix
The methods getAutoCommit, rollback and commit can throw exceptions on stale connections. This PR implements corrected error handling during the connection releasing process so bad connections don't just hang around.

@farmdawgnation farmdawgnation merged commit 3387cdd into lift:master Apr 8, 2014

@rofreytag

This comment has been minimized.

Copy link
Author

rofreytag commented Apr 8, 2014

Wohoo nice!

@rofreytag rofreytag deleted the rofreytag:db_realeasecon_fix branch Apr 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment