TransactionalUndo closes storages it opens.

[jamadden]

Fixes #268.

Additionally, it no longer eagerly opens the new instance, waiting until it actually needs it, and disposes of it (not just closes it) when it is done, thus breaking the reference cycle outlined in #268.

I did the closing in both tpc_finish and tpc_abort because one or the other of those (logically) must be called once tpc_begin was called. In the implementation of transaction, it looks like the plain abort() method is also always called, but the docs don't make it clear that that's required (in fact, they say that abort "may also be called").

[jamadden]

Any interested reviewers?

[jamadden]

I realized we should really be release() the new storage, not closing it. For the MVCC adapters, this doesn't make a difference (they're both no-ops), but on RelStorage there is a difference.

It would be nice to get this merged and the rest of the changes in master released.

[jmuchemb]

Are you sure relstorage needs things to be done when allocating/freeing instances for undos ? I mean: relstorage does not implement undo_instance and mvccstorage does not nothing for undo instances.

If relstorage could implement undo_instance() in such a way calling release would not be needed, the ZODB code would be simpler.

[jamadden]

Are you sure relstorage needs things to be done when allocating/freeing instances for undos ? I mean: relstorage does not implement undo_instance and mvccstorage does not nothing for undo instances.

Yes. RelStorage implements new_instance, which this code calls (RelStorage directly implements IMVCCStorage so it's not ever wrapped in an mvccadapter). That allocates external resources, and those need to be closed in a timely fashion without relying on the garbage collector. Without this change, database connections leak.

If relstorage could implement undo_instance() in such a way calling release would not be needed, the ZODB code would be simpler.

I suppose that's true, but I'm not particularly a fan of that for a few different reasons:

1. It seems like it's just the Right Thing To Do to release() the storage you allocated by calling new_instance(). The responsible party is the one that opened it, they should be the one to close it.
2. Implementing undo_instance in that way would require assuming things about how it is used (e.g., always closing everything at the end of tpc_finish and tpc_abort...just like the code here does, but the code here knows how it's using the storage).
3. undo_instance is not a documented part of any interface, and requiring storages to implement that would need for it to be documented, and then it would push the complexity of knowing/assuming how the storage is used out to all the storage implementations instead of keeping it centralized here.