ZStorm tied too closely to threaded transaction managers

[Gary Poster]

In zstorm's synchronizer, this condition concerns me:

    if store and trans is transaction.get():

Why do we need to verify that the trans is the same as trans.get()? This seems unnecessary to me, and even problematic since it potentially keeps the data manager away from the transaction manager.

In general, I think ZStorm should support explicit (non threaded) transaction managers. They are used and come in handy.

[James Henstridge (jamesh)]

First of all, the zstorm code is explicitly designed to manage per-thread stores, so working with the standard threaded transaction manager seems appropriate. That said, some bits of code could be reused with other transaction managers (e.g. the DataManager implementation).

What sort of API are you looking for, and what sort of problems are you trying to solve?

[Gary Poster (gary)]

On Fri, 2008-10-10 at 10:49 +0000, James Henstridge wrote:
> First of all, the zstorm code is explicitly designed to manage per-
> thread stores, so working with the standard threaded transaction manager
> seems appropriate. That said, some bits of code could be reused with
> other transaction managers (e.g. the DataManager implementation).
>
> What sort of API are you looking for, and what sort of problems are you
> trying to solve?

The API: something to which you can explicitly pass in a transaction
manager, perhaps optionally (it would default to the threaded
transaction manager). This would give you an object fulfilling the
IZStorm interface, tied to the data manager.

The problems:

- It sometimes comes in handy in tests to be able to show how
simultaneous interactions work. Doing this with threads is possible but
comparatively quite painful to simply using separate data managers.
See, for instance, the conflict resolution part (second half) of the
doctest here:

http://pypi.python.org/pypi/zc.queue

- non-threaded use of the API could come in handy when using APIs such
as Twisted's. This is done in my project zc.async with ZODB transaction
use, for instance.

As you can see, the concerns are somewhat theoretical--I don't have a
project that needs this change now in Storm, though I have used a
similar feature before with the ZODB and the transaction module.

The change I propose would definitely provide better compatibility with
the transaction module. I perceive the Zope 3 project, like others, to
be moving towards library reuse, rather than framework reuse. I hope
Storm is interested in that sort of approach as well. What Storm has
now is sufficient for Zope-3-the-framework, but the transaction module
has wider applicability and wider use.

[James Henstridge (jamesh)]

Attached is a minor change to reduce the number of places that explicitly use the default transaction manager to a variable on the ZStorm object. Using an alternative transaction manager should be as simple as using an alternative transaction_manager attribute, and perhaps changing _local to a normal dictionary rather than a threading.local object.

So it should definitely be possible to support this. Adding default arguments to ZStorm's constructor for the transaction manager and a factory for _local would be pretty easy. How does that sound?

[Gary Poster (gary)]

On Wed, 2008-11-05 at 05:37 +0000, James Henstridge wrote:
> Attached is a minor change to reduce the number of places that
> explicitly use the default transaction manager to a variable on the
> ZStorm object. Using an alternative transaction manager should be as
> simple as using an alternative transaction_manager attribute, and
> perhaps changing _local to a normal dictionary rather than a
> threading.local object.
>
> So it should definitely be possible to support this. Adding default
> arguments to ZStorm's constructor for the transaction manager and a
> factory for _local would be pretty easy. How does that sound?

That sounds good, and the patch looks good. If I had a moment, I'd
contribute a test and docs to show how this could work (in particular
the _local replacement, which might not be immediately obvious, nor
clear that it was a documented and supported approach). If you don't
intend to, let me know, and I'll try to fit it in.

[James Henstridge (jamesh)]

A test and some documentation would be useful. As far as the API goes, it'd probably be better to provide a "manage per thread stores" argument to the constructor rather than "factory for the _local attribute".

Even better would be if we could tell whether the ZStorm should be managing per-thread stores from transaction manager itself. I wonder if checking whether it is an instance of ThreadedTransactionManager is enough, or if there might be other subclasses where per-thread stores would be appropriate?

[Gary Poster (gary)]

I'd go so far to say that only bothering with the main threaded transaction manager would be sufficient for the threaded use case. If you disagree, see my first preference, below for a possible story.

For API, I have a first-, second-, and third-place preference.

1) Have a version of ZStorm (name TBD, though I like Stores or StormStores) that requires an explicit transaction manager. ``_local`` is not around: the dicts that normally hang off ``_local`` are on the ZStorm instance itself. Make the current ZStorm a subclass of that, which does not accept a transaction manager (using the threaded transaction manager) and uses _local. If you really think we ought to accept other threaded transaction manager instances or implementations, they could be even acceptable as optional arguments to ZStorm.

2) Alternatively, have a single ZStorm. If you pass in an explicit transaction manager, it assumes you are not threaded, and doesn't use _local. Otherwise, it uses the standard threaded transaction manager and _local.

3) Alternatively, what you said. :-)

[James Henstridge (jamesh)]

I've submitted the patch posted earlier for review, since it seems like a worthwhile clean up in its own right.

Out of the options, (1) sounds like the a good idea: move most of the code to a base class with ZStorm as a specialisation for the threaded transaction manager that uses threading.local().

[James Henstridge (jamesh)]

The initial patch from this bug is now in trunk, which should make implementing the remainder of this bug a bit easier.