Fix transaction manager run problems #58

[d-maurer]

This PR documents attempts and run in transaction.interfaces.ITransactionManager and implements run in terms of attempts (which properly uses transaction manager methods for transaction control). This fixes #58

[d-maurer]

I am not sure about the name != u'_'. Should this simply exclude an empty name (and would then by a typo) or is there a real reason why to handle the name _ specially?

[jamadden]

@jimfulton implemented run back in https://github.com/zopefoundation/transaction/pull/25. At that time there was discussion that attempts() was awkward, and history has shown it to frequently be broken (at least three different times, IIRC). The idea might have been to explicitly not implement run in terms of attempts because of that, though I don't recall (or locate) any discussion to that effect. I think it would be good to get @jimfulton 's input.

[d-maurer]

Building the Python 3.4 travis environment failed. I likely need help to resolve this.

Apparently, "Python 3.4" was not (no longer?) installed and was downloaded. Later pip install -U -e .[test,docs] failed with an exception that indicates a setuptools problem:

Exception:
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.4.6/lib/python3.4/site-packages/pkg_resources/__init__.py", line 2813, in _dep_map
    return self.__dep_map
  File "/home/travis/virtualenv/python3.4.6/lib/python3.4/site-packages/pkg_resources/__init__.py", line 2624, in __getattr__
    raise AttributeError(attr)
AttributeError: _DistInfoDistribution__dep_map

At other places, Python 3.4 support has been dropped. Should we do this here as well? If not, how to resolve the above problem with the travis build environment?

[jamadden]

The underlying error was strange:

  File "/home/travis/virtualenv/python3.4.6/lib/python3.4/site-packages/pkg_resources/__init__.py", line 1577, in _get
    with open(path, 'rb') as stream:
FileNotFoundError: [Errno 2] No such file or directory: '/home/travis/virtualenv/python3.4.6/lib/python3.4/site-packages/six-1.10.0.dist-info/METADATA'

I cleared the build cache and restarted and it went green.

[jugmac00]

@jamadden Could you please give a quick hint how you proceeded in order to clear the build cache?

I am about preparing a blog post on "how to start contributing to the Zope universe" with tipps about travis, tox, and all the various tidbits.

[jamadden]

Clearing caches has some screenshots in the Travis docs. It's very rarely necessary to clear caches in my experience. I suspect that re-running this job would probably have cleared it up without needing to clear the cache, but I was lazy and just began by clearing the cache (the cache for this repo is relatively small and cheap to re-create.)

[mgedmin]

At other places, Python 3.4 support has been dropped. Should we do this here as well?

Yes, but maybe in a separate PR.

[jimfulton]

WRT implementing using attempts, I'm -1 on this. Historically, attempts has been buggy. The implementation of run should be simpler. Having said that I don't have time now to dig into this. I'm willing to defer to people who have time.

I don't have context for name != u'_'.

[d-maurer]

Jason Madden wrote at 2019-5-2 03:55 -0700:

... I cleared the build cache and restarted and it went green.

Thank you for your help!

[d-maurer]

Jim Fulton wrote at 2019-5-2 05:22 -0700:

... I don't have context for name != u'_'.

transaction._manager:

...
    def run(...)
        ...
        name = func.__name__
        doc = func.__doc__

        name = text_(name) if name else u''
        doc = text_(doc) if doc else u''

        if name != u'_':
            if doc:
                doc = name + u'\n\n' + doc
            else:
                doc = name
        ...

[d-maurer]

Marius Gedminas wrote at 2019-5-2 04:46 -0700:

mgedmin commented on this pull request.

Thank you for your valuable comment.

[d-maurer]

I have changed TransactionManager.run back to an elementary implementation (without attempts) - but now using manager methods for transaction control (fixing #58).

interfaces.ITransactionManager now states that a transaction manager can be used as "context manager" to get a transactional context.

Following @mgedmin, I improved the docstring formatting.

[jamadden]

I don't have context for name != u'_'.

That dates back to the original discussion. The run method can be used with decorator syntax to mean "no really, apply this function immediately and return the result instead of the function." Using _ as the name of a variable is a fairly common way to mean "ignore this, it doesn't matter" (e.g., pylint doesn't warn about such things being unused), so here we just ignore functions named that.

[jugmac00]

Clearing caches has some screenshots in the Travis docs. It's very rarely necessary to clear caches in my experience. I suspect that re-running this job would probably have cleared it up without needing to clear the cache, but I was lazy and just began by clearing the cache (the cache for this repo is relatively small and cheap to re-create.)

Thanks a ton! I created a first version of my "how to start contributing to Zope universe guide" (written primarily for myself) at https://jugmac00.github.io/blog/face-to-face-with-earl-zope/

[d-maurer]

Jason Madden wrote at 2019-5-3 11:26 +0000:

jamadden approved this pull request.

Looking pretty good to me. I left some minor comments seeking some clarification.

         try:

result = func()

• txn.commit()
• except Exception as v:
• if i == tries:
• raise # that was our last chance
• retry = self._retryable(v.class, v)
• txn.abort()
• if not retry:
• raise
• else:
• self.commit()

There's a semantic difference here, right?

No.

Previously, if the commit action failed, that failure would be raised to the caller.

This would have been a (severe) bug. Most transactions fail only during their "commit" with a "TransientError".

Fortunately, in both the old as well as the new code, the commit is inside the "try ... except" block.

The differences display confuses you: In the new code, the "return result" is inside the "try: ... except" block; formerly, it was in the "else:" of the "try" (both are semantically equivalent). As a result of this syntactic change, the differences display might be interpreted that the "else:" and the "self.commit()" were close together. But, in fact, they belong to different code versions.

...

@@ -5,7 +5,8 @@ 2.4.1 (unreleased)

-- Nothing changed yet. +- Fix transaction manager run problems

I think this needs to go to version 2.5.0 given the semantic changes.

There are only minor semantic changes:

• the transaction is now aborted also for BaseExceptions which are not Exceptions (e.g. SystemExit and KeyboardInterrupt).

• an empty "name" no longer produces two initial empty lines in the transaction note.

• the "current" transaction is now committed/aborted, not the initial one - this is the main content of #58.

• the error message for "tries <= 0" changed slightly.

I would like to see a more detailed change note, mentioning the commit change discussed below

There is no commit change.

and the fact that the transaction is no longer captured by run but allowed to be changed while run is in progress.

I will replace "fix #58" by "run now commits/aborts the current transaction after the call rather than the initial one".

[jamadden]

The differences display confuses you

You're right, the diff was confusing. I reviewed more carefully the individual files. Thanks for clarifying!

I think this needs to go to version 2.5.0 given the semantic changes.

There are only minor semantic changes:

Agreed (now).

• the transaction is now aborted also for BaseExceptions which are not Exceptions (e.g. SystemExit and KeyboardInterrupt).

That seems also worth mentioning in the change note.

[d-maurer]

The Python 3.4 Traviis build failed again -- looks like the same problem as before (some six related problem without a direct relationship to transaction).

[jugmac00]

The Python 3.4 Traviis build failed again -- looks like the same problem as before (some six related problem without a direct relationship to transaction).

I am about creating a pr to drop Python 3.4 support - you may rebase then.