ClientStorage has buggy implementation of lastTransaction()

[Christian Theune]

The ClientStorage always asks the ZEO cache for the last committed transaction. This at least fails when the cache hasn't seen any entries yet, like right after opening.

The storage API doesn't include a hint that any operation is necessary before calling `lastTransaction` so this should be changed. I can see multiple options:

- make lastTransaction always call `lastTransaction` on the server
- make the cache initialise it's known transaction in the __init__ of ClientStorage

A further thought: is the cache's knowledge about the last transaction *always* the right thing regarding the call to `lastTransaction` or might this be offset due to unprocessed invalidations? Would this violate the storage API?

[Jim Fulton (jim-zope)]

On Jan 10, 2008, at 3:30 AM, Christian Theune wrote:

> Public bug reported:

I agree that this is a bug.

>
>
> The ClientStorage always asks the ZEO cache for the last committed
> transaction. This at least fails when the cache hasn't seen any
> entries
> yet, like right after opening.
>
> The storage API doesn't include a hint that any operation is necessary
> before calling `lastTransaction` so this should be changed. I can see
> multiple options:
>
> - make lastTransaction always call `lastTransaction` on the server
> - make the cache initialise it's known transaction in the __init__
> of ClientStorage

There are going to be times when you can't know the lastTransaction
because you haven't connected to the server yet. Getting the
lastTransaction before it is set should probably raise a disconnected
error.

The cache lastTid should be set during connection and updated by
invalidation.

There's no point in polling the server at other times, since the data
is being sent to us

> A further thought: is the cache's knowledge about the last transaction
> *always* the right thing regarding the call to `lastTransaction` or
> might this be offset due to unprocessed invalidations?

There is always the possibility that the information is out of date.
Networks are not instantaneous. Polling the storage doesn't make the
information more current. I don't think polling will make the
information more timely.

> Would this
> violate the storage API?

No.

Jim

--
Jim Fulton
Zope Corporation

[Christian Theune (ctheune)]

Ok, I'll change this behaviour so that the last tid is updated after opening the connection.

[Christian Theune (ctheune)]

Hey,

Jim Fulton wrote:
> There are going to be times when you can't know the lastTransaction
> because you haven't connected to the server yet. Getting the
> lastTransaction before it is set should probably raise a disconnected
> error.

We're working on this and think that setting the lastTransaction is done best during cache verification. However, the cache currently can not distinguish between not having a transaction set or the transaction being z64.

In my understanding the lastTransaction for a new, otherwise uninitialized, storage should return z64 instead of None. Is that right? This is also touched by MappingStorage and DemoStorage who initializes ltid with None instead of z64.

Should the cache distinguish z64 and None properly to allow differentiating between z64 and the uninitialized state?

[Jim Fulton (jim-zope)]

On Jan 14, 2008, at 4:51 AM, Christian Theune wrote:

> Hey,
>
> Jim Fulton wrote:
>> There are going to be times when you can't know the lastTransaction
>> because you haven't connected to the server yet. Getting the
>> lastTransaction before it is set should probably raise a disconnected
>> error.
>
> We're working on this and think that setting the lastTransaction is
> done
> best during cache verification. However, the cache currently can not
> distinguish between not having a transaction set or the transaction
> being z64.

They are equivalent. A transaction can't have an id if z64.

> In my understanding the lastTransaction for a new, otherwise
> uninitialized, storage should return z64 instead of None. Is that
> right?

I don't know. Looking at the code, I see one almost-certainly-unused
place where lastTransaction is set to return None. I don't really see
any places where anything checks for None being returned, so z64 seems
safest. Whatever we decide, we need to fix the interface be clear on
this.

Note that this is an extreme edge case, as a transaction will be
written as soon as a storage is used by a database.

Note that lastTransaction should raise an error for a disconnected
client storage.

> This is also touched by MappingStorage and DemoStorage who initializes
> ltid with None instead of z64.

Grrr.

> Should the cache distinguish z64 and None properly to allow
> differentiating between z64 and the uninitialized state?

IMO, they are equivalent. We should be consistent in whatever we do.

Jim

--
Jim Fulton
Zope Corporation

[Christian Theune (ctheune)]

Hi,

Jim Fulton schrieb:
> On Jan 14, 2008, at 4:51 AM, Christian Theune wrote:
>> In my understanding the lastTransaction for a new, otherwise
>> uninitialized, storage should return z64 instead of None. Is that
>> right?
>
> I don't know. Looking at the code, I see one almost-certainly-unused
> place where lastTransaction is set to return None. I don't really see
> any places where anything checks for None being returned, so z64 seems
> safest. Whatever we decide, we need to fix the interface be clear on
> this.
>
> Note that this is an extreme edge case, as a transaction will be
> written as soon as a storage is used by a database.

Right. For gocept.zeoraid we talk to the backends after initialization
to find out what their view on the world is, so this edge case is normal
for us.

> Note that lastTransaction should raise an error for a disconnected
> client storage.

Right.

>> This is also touched by MappingStorage and DemoStorage who initializes
>> ltid with None instead of z64.
>
> Grrr.

If we use None as the value (see below) then that would actually be ok,
so the `Grrr` applies to the inconsistency, right? :)

>> Should the cache distinguish z64 and None properly to allow
>> differentiating between z64 and the uninitialized state?
>
>
> IMO, they are equivalent. We should be consistent in whatever we do.

Thomas and I prefer to use None:

a) there was no transaction
b) z64 is not a real transaction ID

So None seems more Pythonic.

I agree that the code should be consistent.

Christian

--
gocept gmbh & co. kg - forsterstrasse 29 - 06112 halle (saale) - germany
www.gocept.com - <email address hidden> - phone [phone] -
fax [phone] - zope and plone consulting and development

[Jim Fulton (jim-zope)]

On Jan 14, 2008, at 10:20 AM, Christian Theune wrote:

> Hi,
>
> Jim Fulton schrieb:
>> On Jan 14, 2008, at 4:51 AM, Christian Theune wrote:
>>> In my understanding the lastTransaction for a new, otherwise
>>> uninitialized, storage should return z64 instead of None. Is that
>>> right?
>>
>> I don't know. Looking at the code, I see one almost-certainly-unused
>> place where lastTransaction is set to return None. I don't really
>> see
>> any places where anything checks for None being returned, so z64
>> seems
>> safest. Whatever we decide, we need to fix the interface be clear on
>> this.
>>
>> Note that this is an extreme edge case, as a transaction will be
>> written as soon as a storage is used by a database.
>
> Right. For gocept.zeoraid we talk to the backends after initialization
> to find out what their view on the world is, so this edge case is
> normal
> for us.
>
>> Note that lastTransaction should raise an error for a disconnected
>> client storage.
>
> Right.
>
>>> This is also touched by MappingStorage and DemoStorage who
>>> initializes
>>> ltid with None instead of z64.
>>
>> Grrr.
>
> If we use None as the value (see below) then that would actually be
> ok,
> so the `Grrr` applies to the inconsistency, right? :)

Right.

>>> Should the cache distinguish z64 and None properly to allow
>>> differentiating between z64 and the uninitialized state?
>>
>>
>> IMO, they are equivalent. We should be consistent in whatever we do.
>
> Thomas and I prefer to use None:
>
> a) there was no transaction
> b) z64 is not a real transaction ID
>
> So None seems more Pythonic.

OK with me as long as you don't break anything. :)

Jim

--
Jim Fulton
Zope Corporation

[Christian Theune (ctheune)]

We worked on this with the goal to replace z64 with `None` when talking about transactions.

We drafted the attached patch with the following comments:

a) The ZEO protocol requires a compatibility API to convert lastTransaction calls that return `None` into `z64` for older clients

b) The store* methods should use `None` instead of z64 for signalling new objects as well.

We know that there are a few tests failing with this patch right now (requires ZODB/trunk revision 83329, not later) but we'd like to get some feedback before proceeding.

[Christian Theune (ctheune)]

Another note: We're fixing the actual issue of this bug (the initial setting of ClientStorage cache's last transaction) in a seperate checkin that maintains the existing inconsistent definition of the lastTransaction API.

[Christian Theune (ctheune)]

A fix was committed on the trunk in revision 83332.

[Christian Theune (ctheune)]

Porting this to the 3.8 branch is a bit hairy, as the short cut for verifying empty caches is missing. The easiest way would be to port the short cut too.

Otherwise we see issues regarding invalidations we might miss when blindly setting the TID for a cache that has entries.