zopefoundation / Products.ZSQLMethods

SQL method support for Zope.
Other
3 stars 14 forks source link

Fix how we join a transaction. #23

Closed viktordick closed 4 years ago

viktordick commented 4 years ago

ITransaction no longer has a method register(), the intended way to register a component in the current transaction seems to be join(). This does not immediately fail due to the unqualified except clause that hides this error, but the effect is that TM objects are no longer correctly rolled back on transaction abort.

I guess this also makes the class Surrogate superfluous, but I'm not deep enough in the code to be sure about this. Also, the unqualified except should probably be rewritten so it only catches expected errors instead of hiding such surprises. But I do not know what the expected errors here are supposed to be.

dataflake commented 4 years ago

Great catch, I'll take a closer look at that today.

dataflake commented 4 years ago

Which database adapter are you using? I am looking at Products.ZMySQLDA and that's not affected because it overrides _register - and uses the correct join call.

dataflake commented 4 years ago

@viktordick Please test the branch and let me know if it fixes your issue. I don't know which context you saw the error in (like a specific database adapter) so I can't verify it in a real-life situation. The only database adapter I work with regularly is not affected because it overrides _register and does the right thing already

viktordick commented 4 years ago

Hi @dataflake and thanks. We are using ZPsycopgDA, although in a forked version that, unfortunately, has run away a bit (https://github.com/perfact/ZPsycopgDA/). But the problem is also present in upstream. I will test the branch. The behavior I was getting with master was that no transaction in PostgreSQL ever got committed, which after some time effectively created very weird locks.

viktordick commented 4 years ago

Sorry, now I get the error:

File "/opt/zope/zope4.1/lib/python3.6/site-packages/Shared/DC/ZRDB/TM.py", line 53, in _register
    self.transaction_manager.get().join(Surrogate(self))
AttributeError: 'DB' object has no attribute 'transaction_manager'

You changed it so that self.transaction_manager is used instead of transaction. I guess this is better because it allows for multiple transaction managers to be used, but the subclasses need to initialize it. Maybe a fallback can be created?

dataflake commented 4 years ago

Please try again, I am now setting transaction_manager at class level. Having that attribute on the instance is a requirement for implementing the transaction.interfaces.IDataManager interface, which is what the join method wants.

viktordick commented 4 years ago

Hi, thanks. Now I am getting

AttributeError: 'Surrogate' object has no attribute 'abort'

If I change the Surrogate(self) into simply self, it works. I guess this class is only necessary for the old register method, not for join?

dataflake commented 4 years ago

I don't know what might break if that isn't there so I tried to leave it untouched. But clearly it can't work because that Surrogate is not an IDataManager. I'll look at it later.

dataflake commented 4 years ago

@viktordick I stopped using that Surrogate class, but since I did not know if any code out there still expects those methods I grafted them onto the TM class itself. Let me know if this works for you.

viktordick commented 4 years ago

@dataflake this works and looks good to me. Thanks for your help!

dataflake commented 4 years ago

Thanks for testing! I'll probably do a release tonight.

dataflake commented 4 years ago

Release 3.5 is now published. I also fixed up the Test tab in the ZMI so it's nicer to use.