Re: [ZODB-Dev] [zopefoundation/ZODB] 49919d: test for POSKeyError during transaction commit
Le 05/02/14 15:44, Jim Fulton a écrit : >When setting a key value pair on a new (not already committed) instance of a >standard BTree, readCurrent method is not called on the connection. This is with your change, right? No, it is without my change. >My understanding is that it is due to the fact that _p_jar and _p_oid are >only set during transaction commit. They can be set earlier by calling the connection add method. This is used often for frameworks that use object ids at the application level. I do not understand what you mean. > >However, with a new BTree instance that also inherits from >Acquisition.Implicit, readCurrent method is called on ZODB connection when >setting key value pair. The only explanation I found is that this instance >_p_jar attribute has a value (acquired in a way or another ?). You could also simulate this by adding an object to a connection using a connection's add method. Wanna update the test to use this technique instead? I'll make a try. But I think I remember having already tried (I have been working on this a few times since september). So if I do not succeed in a reasonable amount of time, I'll come back here. >In this case, when readCurrent is called on an object created during a >savepoint and this savepoint is rolled back, the oid is leftover in the >Connection._readCurrent mapping. This leads to the POSKeyError when >committing later as checkCurrentSerialInTransaction cannot check the object >since it went away at rollback. > >This brings us to the fix I propose: calls to readCurrent should not track >objects with oid equal to z64. ... >This was a very long explanation which I hope will help to confirm the fix >or to come up with a better one. > >PS: keep in mind that english is not my mothertongue. :) You do very well. I think your fix is correct. As you point out, It doesn't make sense to guard against conflicts on new objects. I think a cleaner test could be written using the connection add method. Jim -- Godefroid Chapelle (aka __gotcha) http://bubblenet.be ___ For more information about ZODB, see http://zodb.org/ ZODB-Dev mailing list - ZODB-Dev@zope.org https://mail.zope.org/mailman/listinfo/zodb-dev
Re: [ZODB-Dev] [zopefoundation/ZODB] 49919d: test for POSKeyError during transaction commit
On Wed, Feb 5, 2014 at 9:23 AM, Godefroid Chapelle wrote: > Le 05/02/14 13:25, Jim Fulton a écrit : > > Acquisition is added as a test dependency. Any hint how to replicate > >> >the bug without acquisition is welcome. >> >>Define a subclass of Persistent which emulates what Acquisition does, >> e.g.: >> >> from persistent import Persistent >> class Foo(Persistent): >> @property >> def _p_jar(self): # or whatever attribute trggers >> return object() >>> >>> > >>> >What if full replication requires a C extension module? >>> > >>> >(I hope that's not true and that it is possible to reproduce the bug >>> >using some fakes, but I haven't spent the time investigating this.) >> >> I'm going to dig into this. I'm baffled by the assertion that this has >> anything to do with readCurrent. > > > For sure : the POSKeyError happens during connection.commit when checking > oids stored in Connection._readCurrent mapping. > > (see traceback at http://rpatterson.net/blog/poskeyerror-during-commit) > > The _readCurrent mapping is populated only by calls to > Connection.readCurrent method. > > In the Plone code base, the only way I found to get that > Connection.readCurrent method to be called is by adding a key value pair to > a BTree. > > _BTree_set C function is then called, which in turn calls readCurrent by > inlining the PER_READCURRENT macro. > > This calls the cPersistence.c readCurrent function, which in turn calls > readCurrent method on the ZODB connection. Wow. I had to dig a bit to remind myself (vaguely) why I added this. > > When setting a key value pair on a new (not already committed) instance of a > standard BTree, readCurrent method is not called on the connection. This is with your change, right? > > My understanding is that it is due to the fact that _p_jar and _p_oid are > only set during transaction commit. They can be set earlier by calling the connection add method. This is used often for frameworks that use object ids at the application level. > > However, with a new BTree instance that also inherits from > Acquisition.Implicit, readCurrent method is called on ZODB connection when > setting key value pair. The only explanation I found is that this instance > _p_jar attribute has a value (acquired in a way or another ?). You could also simulate this by adding an object to a connection using a connection's add method. Wanna update the test to use this technique instead? > > In this case, when readCurrent is called on an object created during a > savepoint and this savepoint is rolled back, the oid is leftover in the > Connection._readCurrent mapping. This leads to the POSKeyError when > committing later as checkCurrentSerialInTransaction cannot check the object > since it went away at rollback. > > This brings us to the fix I propose: calls to readCurrent should not track > objects with oid equal to z64. ... > This was a very long explanation which I hope will help to confirm the fix > or to come up with a better one. > > PS: keep in mind that english is not my mothertongue. :) You do very well. I think your fix is correct. As you point out, It doesn't make sense to guard against conflicts on new objects. I think a cleaner test could be written using the connection add method. Jim -- Jim Fulton http://www.linkedin.com/in/jimfulton ___ For more information about ZODB, see http://zodb.org/ ZODB-Dev mailing list - ZODB-Dev@zope.org https://mail.zope.org/mailman/listinfo/zodb-dev
Re: [ZODB-Dev] [zopefoundation/ZODB] 49919d: test for POSKeyError during transaction commit
Le 05/02/14 13:25, Jim Fulton a écrit : Acquisition is added as a test dependency. Any hint how to replicate >> >the bug without acquisition is welcome. >> >>Define a subclass of Persistent which emulates what Acquisition does, e.g.: >> >> from persistent import Persistent >> class Foo(Persistent): >> @property >> def _p_jar(self): # or whatever attribute trggers >> return object() > >What if full replication requires a C extension module? > >(I hope that's not true and that it is possible to reproduce the bug >using some fakes, but I haven't spent the time investigating this.) I'm going to dig into this. I'm baffled by the assertion that this has anything to do with readCurrent. For sure : the POSKeyError happens during connection.commit when checking oids stored in Connection._readCurrent mapping. (see traceback at http://rpatterson.net/blog/poskeyerror-during-commit) The _readCurrent mapping is populated only by calls to Connection.readCurrent method. In the Plone code base, the only way I found to get that Connection.readCurrent method to be called is by adding a key value pair to a BTree. _BTree_set C function is then called, which in turn calls readCurrent by inlining the PER_READCURRENT macro. This calls the cPersistence.c readCurrent function, which in turn calls readCurrent method on the ZODB connection. When setting a key value pair on a new (not already committed) instance of a standard BTree, readCurrent method is not called on the connection. My understanding is that it is due to the fact that _p_jar and _p_oid are only set during transaction commit. However, with a new BTree instance that also inherits from Acquisition.Implicit, readCurrent method is called on ZODB connection when setting key value pair. The only explanation I found is that this instance _p_jar attribute has a value (acquired in a way or another ?). In this case, when readCurrent is called on an object created during a savepoint and this savepoint is rolled back, the oid is leftover in the Connection._readCurrent mapping. This leads to the POSKeyError when committing later as checkCurrentSerialInTransaction cannot check the object since it went away at rollback. This brings us to the fix I propose: calls to readCurrent should not track objects with oid equal to z64. AFAICS, this is inline with readCurrent API goal which is (if I understand well) to ensure that an object manipulated by two connections does not get into an incoherent state. An object that has never been committed cannot be accessed by a second transaction hence does not need to participate in readCurrent dance. This was a very long explanation which I hope will help to confirm the fix or to come up with a better one. PS: keep in mind that english is not my mothertongue. -- Godefroid Chapelle (aka __gotcha) http://bubblenet.be ___ For more information about ZODB, see http://zodb.org/ ZODB-Dev mailing list - ZODB-Dev@zope.org https://mail.zope.org/mailman/listinfo/zodb-dev
Re: [ZODB-Dev] [zopefoundation/ZODB] 49919d: test for POSKeyError during transaction commit
On Wed, Feb 5, 2014 at 1:57 AM, Marius Gedminas wrote: > On Tue, Feb 04, 2014 at 12:44:09PM -0500, Tres Seaver wrote: >> On 02/04/2014 06:28 AM, Godefroid Chapelle wrote: >> > Le 03/02/14 20:53, Tres Seaver a écrit : >> >> I wish you hadn't pushed that -- some of these changes are definitely >> >> inappropriate on the 3.10 branch (adding an Acquisition dependency >> >> is definitely wrong). Agreed. Note that non-trivial commits to a release branch, like master, should be via pull request. >> > >> > Acquisition is added as a test dependency. Any hint how to replicate >> > the bug without acquisition is welcome. >> >> Define a subclass of Persistent which emulates what Acquisition does, e.g.: >> >> from persistent import Persistent >> class Foo(Persistent): >> @property >> def _p_jar(self): # or whatever attribute trggers >> return object() > > What if full replication requires a C extension module? > > (I hope that's not true and that it is possible to reproduce the bug > using some fakes, but I haven't spent the time investigating this.) I'm going to dig into this. I'm baffled by the assertion that this has anything to do with readCurrent. Regardless of whether it should have been made to the 3.10 branch, I'm going to use Godefroid's test case to dig further. > >> > Which other change is inappropriate ? >> >> Adding MANIFEST.in on a release branch seems wrong to me (I don't like >> them anyway, and we *definitely* don't want to encourage >> instsall-from-a-github-generated-tarball on a release branch). > > That's like objecting if someone adds a .gitignore to a release branch. > Or a .travis.yml. It's not code, it's metadata. Yup. > (I never liked setuptool's magic "let me query git to see what source > files you have, but not by default, oh no, instead let's assume > everybody has installed the non-standard plugin into their system > Pythons and then let's silently produce broken tarballs if they haven't, > because obviously implicit is better than explicit, and when there's > temptation the right thing is to guess" behavior anyway, and we > *definitely* don't want broken sdists on PyPI.) I couldn't agree more. One of the advantages of moving to git was circumventing setuptool's misguided magic. I've no idea what Tres was referring to wrt "instsall-from-a-github-generated-tarball", but I use Manifest.in files in all my modern projects. Jim -- Jim Fulton http://www.linkedin.com/in/jimfulton ___ For more information about ZODB, see http://zodb.org/ ZODB-Dev mailing list - ZODB-Dev@zope.org https://mail.zope.org/mailman/listinfo/zodb-dev
