[Zope-dev] kudos...

[Chris McDonough]

On Tue, 2011-08-09 at 11:48 -0400, Zvezdan Petkovic wrote:
> On Aug 8, 2011, at 4:44 PM, Chris McDonough wrote:
> 
> > Kudos to whomever turned the "transaction" package's transaction manager into a context manager and was thoughtful enough to provide the "attempts" method (which returns a separate context manager, wrapping the txn context manager, and retries some number of configurable times).
> > This makes writing code that integrates with a web system that commits
> > or aborts as easy as:
> > 
> >         import transaction
> > 
> >         for attempt in transaction.attempts(attempts):
> >            with attempt as t:
> >                response = handler(request)
> > 
> >                if t.isDoomed():
> >                    t.abort()
> > 
> >                if commit_veto is not None:
> >                    veto = commit_veto(request, response)
> >                    veto and t.abort()
> > 
> >                return response
> > 
> > Very nice.
> > 
> > - C
> 
> 
> Yes, this is *very* convenient and I like to use it, but beware of the bug (it's not affecting you above because of the return).
> 
> According to the documentation, it's intended as a replacement for:
> 
>     for i in range(3):
>         try:
>            with transaction:
>                ... some something ...
>         except SomeTransientException:
>            continue
>         else:
>            break
> 
> So, presumably,
> 
>     for attempt in transanction.attempts(retries):
>         with attempt as tx:
>             do_something()
> 
> should break after the *first* successful commit.
> 
> It doesn't.  Instead, it commits do_something() ``retries`` times which is most probably not intended.

Not sure about that; I just return a value if it succeeds.  I wouldnt
expect it to know to break out of the loop without a return in there.

I wound up with this, FWIW:

def tm_tween_factory(handler, registry, transaction=transaction):
    # transaction parameterized for testing purposes
    commit_veto = registry.settings.get('pyramid_tm.commit_veto',
                                        default_commit_veto)
    attempts = int(registry.settings.get('pyramid_tm.attempts', 1))

    if not commit_veto:
        commit_veto = None
    else:
        commit_veto = resolver.maybe_resolve(commit_veto)

    def tm_tween(request):
        if 'repoze.tm.active' in request.environ:
            # don't handle txn mgmt if repoze.tm is in the WSGI pipeline
            return handler(request)

        try:
            for attempt in transaction.attempts(attempts):
                with attempt as t:
                    # make_body_seekable will copy wsgi.input if
                    # necessary, otherwise it will rewind the copy
                    # to position zero
                    if attempts != 1:
                        request.make_body_seekable()
                    response = handler(request)
                    if t.isDoomed():
                        raise AbortResponse(response)
                    if commit_veto is not None:
                        veto = commit_veto(request, response)
                        if veto:
                            raise AbortResponse(response)
                    return response
        except AbortResponse, e:
            return e.response

    return tm_tween

A little more verbose but still nice.

> 
> See https://bugs.launchpad.net/transaction/+bug/724332
> The patch attached to the bug report is just a test that proves that behavior is unexpected and gives a workaround.
> 
> The workaround is simple enough and I prefer to use transaction.attempts convenience rather than the typical try-except code above.
> 
> Still, we really should find time to fix this in the transaction manager code so that it behaves as expected. (Or change the documentation and tests).
> 
> Regards,
> 
>     Zvezdan
> 
> _______________________________________________
> Zope-Dev maillist  -  Zope-Dev at zope.org
> https://mail.zope.org/mailman/listinfo/zope-dev
> **  No cross posts or HTML encoding!  **
> (Related lists - 
>  https://mail.zope.org/mailman/listinfo/zope-announce
>  https://mail.zope.org/mailman/listinfo/zope )
>