mongoengine
mongoengine copied to clipboard
MongoEngine
Transactions via an internal global session var
An implementation of one of my proposals, resolves #2248 - Add support for transactions using global sessions
- Maintain state of session when entering and exiting transaction
- Update any collection methods used in mongoengine that accept session
- Update any database methods used in mongoengine that accept session
- Cascades should be accounted for as those methods are wrappers which call the same low-level pymongo collection APIs updated above
- Any signals that try to read or write should also be wrapped in the transaction
Something important for us to keep in mind: pymongo is extremely upfront that sessions are neither fork-safe nor thread-safe.
At minimum the docs would need to mirror this, but from an implementation stand point, minor protections could be added here by leveraging python thread's local storage.
That said...it wouldn't actually provide any real protections in a threaded/forked environment, given that behind the scenes pymongo is still the one in control.
OK - I see what's going on - per the docs
In MongoDB 4.2 and earlier, you cannot create collections in transactions. Write operations that result in document inserts (e.g. insert or update operations with upsert: true) must be on existing collections if run inside transactions.
Starting in MongoDB 4.4, you can create collections in transactions implicitly or explicitly.
So - the test suite is accurate! :smile: I'm testing against a more recent version of Mongo, which is why the tests pass locally.
I think it makes sense to bubble up the exception to a user that they're violating the expectations of the version of Mongo they're using. Should I just have a conditional assertion in my tests that this case should throw an exception for versions<4.4 but >=4.4 the operation is allowed and successful?
Apologize for the delay, I had a first look at the proposal and it seems to be going in the right direction.
Biggest problem I see right now is that it is not thread safe, using the run_in_transaction context manager in 2 different threads would be overwriting the global _session, and would be mixing things up badly.
From what I understand the Session is indeed documented as not thread safe, so you can't share a same Session among different threads, but you can let different threads instantiate their own session. So using thread locals should do the trick here. Each thread would create its own Session as needed. We would also need a test that simulates this (I would mix ~ 10 threads, with random delays, some of them saving objects, some of them aborting due to an exception), then assert at the end that only expected items were saved.
No apologies necessary! Thanks for the feedback - I'll try to address them as soon as I can.
Great idea about thread locals, btw!
Great code, but why not add arguments read concern and write concern? That would be more flexible
Thank you for the feedback @EVA-08 - I've added at minimum a placeholder for users to provide a dict of kwargs they might need for start_session or start_transaction
We'll see if the maintainers are OK with this.
Would it be possible to get this PR reviewed? Would be really nice to have transactions available in mongoengine
CC @bagerard
Thank you so much for the feedback @ShaneHarvey
Great call about using with_transaction. If you're OK with it, I might just rewrite it now to use with_transaction. Really the "rewrite" will be in the tests to adhere to the new contract. This feels like it might be the right way to kick off transactions in mongoengine - it's cleaner...unlike that unpleasant test I wrote :)
I do see an implication with this approach: if you want to use mongoengine transactions, you have to be using pymongo>=3.9 - is that OK?
I believe I've addressed the feedback provided, but do let me know if anything else comes to mind.
I'm more than happy to do a follow up PR modifying the API to use with_transaction as I completely agree that it is going to be significantly more friendly.
My only concern with doing it as a follow up is that it would be a non-backwards compatible change and the the implication I mentioned above (pymongo>=3.9 is a requirement).
Looking forward to your thoughts!
This PR https://github.com/MongoEngine/mongoengine/pull/2768 Fix the issue of transaction session and transaction between DB
If you're OK with it, I might just rewrite it now to use with_transaction. Really the "rewrite" will be in the tests to adhere to the new contract. This feels like it might be the right way to kick off transactions in mongoengine - it's cleaner...unlike that unpleasant test I wrote :)
Yes I totally agree that it's better done in this PR to avoid breaking changes or code churn.
I do see an implication with this approach: if you want to use mongoengine transactions, you have to be using pymongo>=3.9 - is that OK?
This is okay. You could even check the pymongo version at runtime (like mongoengine does here) and give a nice error message if you like.
I've completed the modifications so that we now abstract with_transaction as opposed to start_transaction. If run_in_transaction is called and pymongo<3.9 is being used, we raise a mongoengine OperationError and unit tests are skipped when pymongo<3.9 is active.
The other additional change I made was to move run_in_transaction out of context_managers.py because it's no longer a context manager. Instead, I put it closer to the newly added session stuff in connection.py (and updated the tests accordingly, as well).
Let me know if anything else comes to mind!
again apologize for the delay, I'm finally getting back to this.
@ShaneHarvey I find the "callback api" (i.e session.with_transaction(callback)) really awkward, this is typically what context managers are for...
I agree that the retry logic is great to abstract from end users but Pymongo should provide the most flexible and pythonic approach. Using callback and lambda's also interfer with mypy so unless I'm missing something it really provides no advantages...
In fact if Pymongo would offer the "context manager API" (that would include the retry logic), we could easily write a "callback" wrapper in just a few lines of Python but we can't do the opposite and derive a context manager out of the "callback api" which is a pity...
@bagerard Thank you for the feedback.
Would you prefer the implementation I had prior to the callback modification - or is there something else you have in mind?
PyMongo does have a context manager API but the drawback is that is not possible to have retries with such an API as mentioned above. Retries are not possible because the context block can only ever be run once; a context manager can not re-run the block it contains. This is why we built the with_transaction api that accept a callback that can be executed multiple times if needed.
I missed that it was actually re-calling the method :grimacing: , I understand now, thanks for (re)-clarifying.
In practice when you just chain a bunch of pure MongoDB operations, using the callback thing is a perfect fit. When used in a more involved application, like a complex web application (which is what we are building on top of MongoEngine at work), I don't think it would be a sane default to just retry a chain of calls, simply failing the overall transaction and API call sounds like a much much safer thing to do (otherwise you must be careful that first execution is not going to leave stuff behind that could interfer with the second execution). Other obvious issue with this automatic re-call, is that pymongo is silently doing that, without even logging the error or the fact that it's retrying :grimacing: , again makes sense for a chain of pure db operations, but a nightmare to debug in a more complex setup.
Long story short, I think it's a must to provide a simple context manager API for opening the transaction in MongoEngine, without any default retry logic. Next to that we can also support the "callback-api" but it's a nice to have IMO.
I don't think it would be a sane default to just retry a chain of calls
I understand this concern but I don't think this is as big of a concern as you say since the primary use case for transaction is a bunch of pure MongoDB operations. Doing other work inside the transaction extends its lifespan and increases the chance of write conflicts. Also, basically all users will need to write retry logic so the same problem still exists in either API.
Other obvious issue with this automatic re-call, is that pymongo is silently doing that, without even logging the error or the fact that it's retrying 😬 , again makes sense for a chain of pure db operations, but a nightmare to debug in a more complex setup.
We are actually working on adding logging into PyMongo right now: https://jira.mongodb.org/browse/PYTHON-3113
The huge benefit to using the callback approach is that it makes applications much more resilient to transient errors (which are very common when using transactions). Users will be much less likely to have to write custom and error prone transaction retry logic. By opting for a context api you'll end up with a lot of users scratching their heads diagnosing transient write conflict errors.
I would say that if we do end up adding two APIs, then the callback based one should still be the recommended approach in the docs and examples. The context based one could be a "power user" feature for those who want to customize their own retry behavior.
One other idea: if you're very concerned about the retry behavior, one alternative is to expose a configurable knob like this:
run_in_transaction(callback, retry_enabled=False/True)
That would at least let you expose a single txn api instead of 2 different ones. I'm ambivalent but it's worth a thought.
I'm expecting MongoEngine to be used mostly in web applications and users will be opening their transactions quite early, wrapping all their business logic. In the middle of the chain of calls we may have disk operations, network calls, use of globals/threads/threadlocals, stateful objects, etc. Safe repetition of call is not something you get out of the box so I can't recommend that setup in a general purpose ORM like MongoEngine.
That being said, I don't have much experience with transactions in Mongo, is there anything in MongoDB transactions that is different from SQL transactions and that would make them fail more often? We don't see such callback api with repeated calls for dealing with transactions in extremely popular framework like SQLAlchemy or Django, how do you explain this?
Hi yall. is there any update on this MR? Is there anything I can do to help push this along?
Hi yall. is there any update on this MR? Is there anything I can do to help push this along?
I was waiting to see if there would be a response to @bagerard 's last question - it seemed to me that we were still waiting on alignment. All that said: @bagerard to keep things moving, I can roll this back to a state that I think reflects more of what you were expecting wrt "simple context manager without retry logic". More than happy to get that done.
The test cases also include examples of how a user would have to deal with retrying transaction failures.