Skip to content

Commit

Permalink
Changed DB root-object-initialization to use an open connection and more
Browse files Browse the repository at this point in the history
This fixes #84.

Also:

- Added missing transaction ``begin`` call in the DB ``transaction()``
  context manager.

- Added the ability to add a transaction note whan calling
  ``transaction()``.  This is useful (as would be the ability to pass
  in other transaction meta data, but this was needed by (and this
  tested) to make an existing test pass.
  • Loading branch information
Jim Fulton committed Jul 23, 2016
1 parent dedd5a2 commit 03b8183
Show file tree
Hide file tree
Showing 8 changed files with 63 additions and 44 deletions.
24 changes: 13 additions & 11 deletions src/ZODB/Connection.py
Expand Up @@ -190,7 +190,6 @@ def before_instance(before):

self._reader = ObjectReader(self, self._cache, self._db.classFactory)


def add(self, obj):
"""Add a new object 'obj' to the database and assign it an oid."""
if self.opened is None:
Expand All @@ -202,19 +201,22 @@ def add(self, obj):
raise TypeError("Only first-class persistent objects may be"
" added to a Connection.", obj)
elif obj._p_jar is None:
assert obj._p_oid is None
oid = obj._p_oid = self.new_oid()
obj._p_jar = self
if self._added_during_commit is not None:
self._added_during_commit.append(obj)
self._register(obj)
# Add to _added after calling register(), so that _added
# can be used as a test for whether the object has been
# registered with the transaction.
self._added[oid] = obj
self._add(obj, self.new_oid())
elif obj._p_jar is not self:
raise InvalidObjectReference(obj, obj._p_jar)

def _add(self, obj, oid):
assert obj._p_oid is None
oid = obj._p_oid = oid
obj._p_jar = self
if self._added_during_commit is not None:
self._added_during_commit.append(obj)
self._register(obj)
# Add to _added after calling register(), so that _added
# can be used as a test for whether the object has been
# registered with the transaction.
self._added[oid] = obj

def get(self, oid):
"""Return the persistent object with oid 'oid'."""
if self.opened is None:
Expand Down
49 changes: 20 additions & 29 deletions src/ZODB/DB.py
Expand Up @@ -39,6 +39,7 @@
from persistent.TimeStamp import TimeStamp
import six

from . import POSException

logger = logging.getLogger('ZODB.DB')

Expand Down Expand Up @@ -211,7 +212,8 @@ def availableGC(self):
"""Perform garbage collection on available connections.
If a connection is no longer viable because it has timed out, it is
garbage collected."""
garbage collected.
"""
threshhold = time.time() - self.timeout

to_remove = ()
Expand Down Expand Up @@ -442,30 +444,6 @@ def __init__(self, storage,
DeprecationWarning, 2)
storage.tpc_vote = lambda *args: None

temp_storage = self._mvcc_storage.new_instance()
try:
try:
temp_storage.poll_invalidations()
temp_storage.load(z64)
except KeyError:
# Create the database's root in the storage if it doesn't exist
from persistent.mapping import PersistentMapping
root = PersistentMapping()
# Manually create a pickle for the root to put in the storage.
# The pickle must be in the special ZODB format.
file = BytesIO()
p = Pickler(file, _protocol)
p.dump((root.__class__, None))
p.dump(root.__getstate__())
t = transaction.Transaction()
t.description = 'initial database creation'
temp_storage.tpc_begin(t)
temp_storage.store(z64, None, file.getvalue(), '', t)
temp_storage.tpc_vote(t)
temp_storage.tpc_finish(t)
finally:
temp_storage.release()

# Multi-database setup.
if databases is None:
databases = {}
Expand All @@ -479,6 +457,15 @@ def __init__(self, storage,

self.large_record_size = large_record_size

# Make sure we have a root:
with self.transaction('initial database creation') as conn:
try:
conn.get(z64)
except KeyError:
from persistent.mapping import PersistentMapping
root = PersistentMapping()
conn._add(root, z64)

@property
def _storage(self): # Backward compatibility
return self.storage
Expand Down Expand Up @@ -906,8 +893,8 @@ def undo(self, id, txn=None):
"""
self.undoMultiple([id], txn)

def transaction(self):
return ContextManager(self)
def transaction(self, note=None):
return ContextManager(self, note)

def new_oid(self):
return self.storage.new_oid()
Expand All @@ -926,12 +913,16 @@ class ContextManager:
"""PEP 343 context manager
"""

def __init__(self, db):
def __init__(self, db, note=None):
self.db = db
self.note = note

def __enter__(self):
self.tm = transaction.TransactionManager()
self.tm = tm = transaction.TransactionManager()
self.conn = self.db.open(self.tm)
t = tm.begin()
if self.note:
t.note(self.note)
return self.conn

def __exit__(self, t, v, tb):
Expand Down
3 changes: 3 additions & 0 deletions src/ZODB/tests/dbopen.txt
Expand Up @@ -126,6 +126,7 @@ returned are distinct:
>>> st = Storage()
>>> db = DB(st)
>>> c1 = db.open()
>>> c1.cacheMinimize()
>>> c2 = db.open()
>>> c3 = db.open()
>>> c1 is c2 or c1 is c3 or c2 is c3
Expand Down Expand Up @@ -260,6 +261,7 @@ closed one out of the available connection stack.
>>> st = Storage()
>>> db = DB(st, pool_size=3)
>>> conns = [db.open() for dummy in range(6)]
>>> conns[0].cacheMinimize()
>>> len(handler.records) # 3 warnings for the "excess" connections
3
>>> pool = db.pool
Expand Down Expand Up @@ -328,6 +330,7 @@ resources (like RDB connections), for the duration.
>>> st = Storage()
>>> db = DB(st, pool_size=2)
>>> conn0 = db.open()
>>> conn0.cacheMinimize() # See fix84.rst
>>> len(conn0._cache) # empty now
0
>>> import transaction
Expand Down
19 changes: 19 additions & 0 deletions src/ZODB/tests/fix84.rst
@@ -0,0 +1,19 @@
A change in the way databases were initialized affected tests
=============================================================

Originally, databases added root objects by interacting directly with
storages, rather than using connections. As storages transaction
interaction became more complex, interacting directly with storages
let to duplicated code (and buggy) code.

See: https://github.com/zopefoundation/ZODB/issues/84

Fixing this had some impacts that affected tests:

- New databases now have a connection with a single object in it's cache.
This is a very slightly good thing, but it broke some tests expectations.

- Tests that manipulated time, had their clocks off because of new time calls.

This led to some test fixes, in many cases adding a mysterious
``cacheMinimize()`` call.
1 change: 1 addition & 0 deletions src/ZODB/tests/synchronizers.txt
Expand Up @@ -21,6 +21,7 @@ Make a change locally:

>>> st = SimpleStorage()
>>> db = ZODB.DB(st)
>>> st.sync_called = False
>>> cn = db.open()
>>> rt = cn.root()
>>> rt['a'] = 1
Expand Down
3 changes: 2 additions & 1 deletion src/ZODB/tests/testCache.py
Expand Up @@ -237,8 +237,8 @@ def testLRU(self):
# not bother to check this

def testSize(self):
self.db.cacheMinimize()
self.assertEqual(self.db.cacheSize(), 0)
self.assertEqual(self.db.cacheDetailSize(), [])

CACHE_SIZE = 10
self.db.setCacheSize(CACHE_SIZE)
Expand Down Expand Up @@ -444,6 +444,7 @@ def test_basic_cache_size_estimation():
>>> import ZODB.MappingStorage
>>> db = ZODB.MappingStorage.DB()
>>> conn = db.open()
>>> conn.cacheMinimize() # See fix84.rst
>>> def check_cache_size(cache, expected):
... actual = cache.total_estimated_size
Expand Down
1 change: 1 addition & 0 deletions src/ZODB/tests/testConnection.py
Expand Up @@ -230,6 +230,7 @@ def doctest_get(self):
>>> db = databaseFromString("<zodb>\n<mappingstorage/>\n</zodb>")
>>> cn = db.open()
>>> cn.cacheMinimize() # See fix84.rst
>>> obj = cn.get(p64(0))
>>> obj._p_oid
'\x00\x00\x00\x00\x00\x00\x00\x00'
Expand Down
7 changes: 4 additions & 3 deletions src/ZODB/tests/testDB.py
Expand Up @@ -125,7 +125,7 @@ def connectionDebugInfo():
r"""DB.connectionDebugInfo provides information about connections.
>>> import time
>>> now = 1228423244.5
>>> now = 1228423244.1
>>> def faux_time():
... global now
... now += .1
Expand Down Expand Up @@ -154,8 +154,8 @@ def connectionDebugInfo():
>>> before = [x['before'] for x in info]
>>> opened = [x['opened'] for x in info]
>>> infos = [x['info'] for x in info]
>>> before
[None, '\x03zY\xd8\xc0m9\xdd', None]
>>> before == [None, c1.root()._p_serial, None]
True
>>> opened
['2008-12-04T20:40:44Z (1.30s)', '2008-12-04T20:40:46Z (0.10s)', None]
>>> infos
Expand Down Expand Up @@ -351,6 +351,7 @@ def minimally_test_connection_timeout():
>>> db = ZODB.DB(None, pool_timeout=.01)
>>> c1 = db.open()
>>> c1.cacheMinimize() # See fix84.rst
>>> c2 = db.open()
>>> c1.close()
>>> c2.close()
Expand Down

7 comments on commit 03b8183

@bloodbare
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimfulton ouch! It breaks our build because we don't have thread transactions, we are getting the transaction from the frame looking for a Request/View object.

@jimfulton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bloodbare I don't understand what you're trying to say. Can you unpack that please?

@bloodbare
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimfulton on the experimental plone.server (https://github.com/plone/plone.server.git) we have a RequestAwareDBConnection, so objects get registered on a data manager on the request object, not on a local thread var in order to allow mutiple transactions/thread. On the CMS REST API is supposed that all operations will be tied to a request object, minus the creation of the root object that is created on startup of the application. We can wrap the creation of the DB with a Request object so it works or add the specific use case on _register if its the root object, but we didn't need it before because the root was not transaction managed :)

@jimfulton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bloodbare I don't see how this change affects what you describe.

  1. Creation of the root object was always done in a transaction. The DB object acted as the transaction manager.
  2. Now, DB used the transaction method to open a connection with a dedicated transaction manager. The transaction manager isn't tied to a thread in any way. If this is breaking your code, it's almost certainly exposing a latent bug in your code. (This did expose a bug in ZEO, which was recently fixed, where invalidations weren't sent to clients for transactions, like this one, that only added objects.)

If you want to discuss, I'm often in #zodb on on freenode.

@jimfulton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From irc:
bloodbare, a) why would database initialization using a standard tm break this and b) why don't you just hang a standard TM on a request.
In fact, there are existing tools that do this for you.
For example, zc.zodbwsgi attaches a ZODB connection and has an option for that connection to use a transaction manager that's not connected to a thread.
In fact: https://github.com/zc/twotieredkanban/blob/master/buildout.cfg illustrates using this option for a ZODB web app that uses gevent.

@bloodbare
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the missing point is that we store the transaction and the datamanager on the request., and that we are reusing the connection for all the requests. This last feature it will be configurable (to open connection/request or reuse), so there is less RAM on using caches. Reusing the connection makes us to lose MVCC and transaction on the connection but we can have multiple transactions on the same thread.

the main file for this subject is :

https://github.com/plone/plone.server/blob/master/src/plone.server/plone/server/transactions.py

Sorry maybe we are missing something we don't know!

@jimfulton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that you store the transaction and data manager on the request. As I pointed out, that's easily done without any custom software.

"Reusing the connection makes us to lose MVCC" WTF? (Maybe I'll pretend you didn't say that. :) )

In any case, the change to ZODB shouldn't really affect your code other than when you ask for a connection the first time, you may get a connection that was used to add the root object. If this somehow reveals breakage, the breakage was already there waiting to be revealed.

Please sign in to comment.