Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document why newTransaction is only called by begin. #40

Merged
merged 2 commits into from Dec 15, 2016

Conversation

jimfulton
Copy link
Member

No description provided.

Copy link
Member

@jamadden jamadden left a comment

Choose a reason for hiding this comment

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

One small typo. Some possibilities for clarification that don't necessarily have to be addressed here.

passing the new transaction object.

Note that transactions may be started implicitly without
calling ``begin``. In that case, ``newTransaction`` isn't
Copy link
Member

Choose a reason for hiding this comment

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

"without calling begin, for example, by calling get when there is no transaction in progress."

That's not really right, either, though, because what I think you're referring to is different than a Transaction as managed by a manager. See my comment below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. The fact that get creates a transaction object is an implementation detail. get doesn't create a transaction. :)

Copy link
Member

Choose a reason for hiding this comment

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

I suspect that if we thought of them as one and the same and treated them as such this would be a lot less confusing :) (But maybe that's just because that's what I used to do in my own closed-source Java transaction package.)

Copy link
Member Author

Choose a reason for hiding this comment

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

We do think of them as one and the same, but the current behavior betrays that model.

Note that transactions may be started implicitly without
calling ``begin``. In that case, ``newTransaction`` isn't
called because the transaction manager doesn't know when to
call it. The transaction is likely to have begin long before
Copy link
Member

Choose a reason for hiding this comment

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

begin -> begun

Copy link
Member

Choose a reason for hiding this comment

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

Actually, is there a way other than using get to create a transaction? It seems to me that the transaction manager is always involved in starting transactions. But what it may not be involved in is work done outside the actual transaction boundaries.

Perhaps there's a terminology problem here. There's an actual Transaction object, and the transaction manager is always aware of when that begins (because it creates it). But there are people that do work before a Transaction has begun, and would like that work to be reflected in an implicitly begun Transaction; they think of all the work they do as a logical transaction (little t). This would be fine, except there may be synchronizers that do "destructive" things when a Transaction is begun (ZODB polling being a prominent example).

The short answer is "you're doing it wrong" if you expect that to work, IMHO. This is a logical inconsistency in the API and the way that Transactions work.

Can we deprecate the implicit-begin behaviour of get(), such that sometime in the future it either returns None if no transaction has begun (which will make debugging the issue easier, callers will get exceptions immediately if they assume a transaction can just be started) or calls newTransaction if there is no transaction (potentially less invasive, but also potentially harder to debug)? This is probably the wrong place to discuss this...

Copy link
Member Author

Choose a reason for hiding this comment

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

get doesn't really create a transaction. It creates an object that represents a transaction. As you point out, they are separate.

begin creates a transaction object as well.

begin is an application's opportunity to play nice and let everyone know what's going on.

I'm not proud of the implicit transaction model. If I had it do do over, I'd do it differently. (I think I did it this way originally based on experience with Ingres and/or Oracle.)

I'll also note that implicit behavior is useful for informal usage. In fact, I could see use for a sort of an auto-commit mode that only allowed reads, although I'm not sure if it could be done safely (because it could lead to invalidation of objects mid-computation).

In any case, allowing an explicit mode is an important first step. (And outside the scope of this pr. :))

@jimfulton jimfulton merged commit 7838b4c into master Dec 15, 2016
@jimfulton jimfulton deleted the wtf-newTransaction branch December 15, 2016 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants