It's very easy to create reference cycles that require the GC to clean up

[jamadden]

As discussed in zopefoundation/ZODB#268 (comment), if, through any arbitrarily long chain of imports, a data manager implementation ultimately winds up importing the transaction module, then when it joins the default transaction there will be a reference cycle (the data manager will reference the transaction module through __globals__, which references the transaction manager, which references the transaction which references the data manager).

I'm not sure what, if anything, can be done about this, but if anybody has an idea, that'd be great.

[d-maurer]

Jason Madden wrote at 2019-5-14 06:24 -0700:

As discussed in zopefoundation/ZODB#268 (comment), if, through any arbitrarily long chain of imports, a data manager implementation ultimately winds up importing the `transaction` module, then when it joins the default transaction there will be a reference cycle (the data manager will reference the `transaction` module through `__globals__`, which references the transaction manager, which references the transaction which references the data manager). I'm not sure what, if anything, can be done about this, but if anybody has an idea, that'd be great.

The transaction could (and I think does normally) break its connection to the data (aka resource) managers in a commit/abort - thus breaking the cycle. I would expect also that a transaction interacts with its transaction manager on commit/abort. This, too, could be a place to break a potential cycle. That reminds me that after a commit/abort, transaction hooks should get cleared as well - as they might have references to a data manager as well (even though, a data manager does not need hooks to interact with the transaction).

[jamadden]

The transaction could (and I think does normally) break its connection to the data (aka resource) managers in a commit/abort - thus breaking the cycle.

That's an excellent point. Transaction.commit() calls Transaction._free() (which unjoins all the IDataManagers with del self._resources) in the case that no exception is raised by a resource manager. That should break the obvious cycle.

But in this particular test case, the resource warning doesn't show up until the garbage collector runs, and it always shows up exactly when you run the collector, which (I think) means that there was a cycle involved:

//lib/python3.7/site-packages/zope/testrunner/runner.py:408: ResourceWarning: unclosed <socket.socket fd=9, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 55893), raddr=('127.0.0.1', 5432)>
  gc.collect()

There must be some other cycle...

[d-maurer]

@jamadden

...Transaction.commit() calls Transaction._free() (which unjoins all the IDataManagers with del self._resources) in the case that no exception is raised by a resource manager. ...

Looks like we had a bug here: _free should be called, too, when _commitResources raises an exception and not only when it succeeds (maybe same thing for the synchronizers). However, this is likely not the cause for the observed cycle.

[d-maurer]

Looks like we had a bug here: _free should be called, too, when _commitResources raises an exception and not only when it succeeds (maybe same thing for the synchronizers). However, this is likely not the cause for the observed cycle.

Likely a feature (not a bug): not calling _free facilitates the analysis of potential problems (transaction and resource managers are still available for analysis); the transaction is (hopefully) doomed and will get finished/cleaned up by a following abort.

[zopyx]

Feature or bug...the ResourceWanring issues should be somehow handled. It is not very helpful and not trustworthy for people running Zope or Plone in production with tons of such messages in the logs for standard operations.

[jamadden]

I had a branch where I tested doing more cleanups (having _free() drop its reference to _manager and _synchronizers, among other things) and it didn't make a difference. I'm fully convinced the cycle is elsewhere, so I'm going to close this issue.

I decided not to pursue that branch into a PR because (a) it didn't fix the problem and (b) there were some subtle semantic differences. Notably, in the abort() case, currently the synchronizer afterCompletion methods were called after the transaction had been freed (meaning they could begin another one) --- in the commit() case, currently the afterCompletion methods were called before the transaction had been freed. The branch made that consistent, always calling them before the transaction had been freed, but it's not clear to me that was correct or desired (even though all of RelStorage's tests pass with that change). I'll leave that branch around for awhile in case anyone does want to pursue it.

I'd like to make a release of transaction 3.0 to get the new hook functionality out. If there are no objections I'll plan on doing that in a day or so.