Add source exception when raising InvalidRequestError 'Can't operate on closed transaction inside context manager' in _trans_ctx_check #11243
Replies: 3 comments 7 replies
-
|
Hi, Looking at Can you create an example? |
Beta Was this translation helpful? Give feedback.
-
|
agree, that method has no relationship to an exception that was thrown. here's how this is tested in code: with session.begin():
# do things
# rollback() or commit() inside the block.
session.rollback()
# this raises an exception
session.add(u3)I am not seeing what arrangement would have an exception being thrown that would not be part of the context. like if you had this: with session.begin():
# do things
try:
# do some other thing
except Exception as e:
# oh no
session.rollback()
# still using the session inside the block! this raises the
# exception in question. however, "e" above **will** be listed
# in the traceback
session.add(u3)still no issue |
Beta Was this translation helpful? Give feedback.
-
|
Sorry, should have included a code example in the original post 😅 This is the scenario, I'm referring to with session.begin():
# do things
try:
# do some other thing
except Exception as e:
# oh no
session.rollback()
# still using the session inside the block! this raises the
# exception in question.
session.add(u3)Obviously the above code is overly simplistic, a more real world example, although still contrived would be something along the lines of: def _try_doing_something(session):
try:
# do that thing which might fail
except Exception as e:
# oh no
session.rollback()
def do_multiple_things_in_transaction():
with session.begin():
_try_doing_something(session)
session.add(u3)
# We now have an InvalidRequestError,
# which doesn't link to the original exceptionNow, the So putting all together can look kind of like @classmethod
def _trans_ctx_check(cls, subject):
trans_context = subject._trans_context_manager
if trans_context:
if not trans_context._transaction_is_active():
raise exc.InvalidRequestError(
"Can't operate on closed transaction inside context "
"manager. Please complete the context manager "
"before emitting further commands."
) from subject.transaction._rollback_exception |
Beta Was this translation helpful? Give feedback.
-
|
in your first code example you caught the exception. then you call session.add() outside of the block. the exception is gone and has been garbage collected. so you need to log that exception on your end while you're inside the except block. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @zzzeek, Thank you again for your time Please don't get me wrong, I don't think the code I've provided represents a good practice or anything like that. From the perspective of an SQLAlchemy user for the past 4 years, it is abundantly clear that you've put some real effort into making the error messages as helpful as they can possibly be, providing a runtime documentation through validation sort of experience. that is basically to say, that it feels that SQLAlchemy knows it's gonna be used wrong at times and when it does, points to the root cause and the way to fix the underlying issue. Locally changing sqlalchemy code in the manner is the thing which that helped me identify the offending piece of code causing the original faulty behavior (As you can imagine, it was catching and suppressing a unique violation) Why would the exception already be garbage collected if we are still inside the transaction context manager, |
Beta Was this translation helpful? Give feedback.
-
|
My two cents here: I'm not sure if using Thoughts on this? |
Beta Was this translation helpful? Give feedback.
-
I did not claim your code had any kind of bad practice in it, I was only noting that since the code has left the "except:" block, the exception and traceback is gone and your feature request appeared to be impossible. However, since you just mentioned it, I see there is actually a I would support only augmenting the message in the new exception, and/or making a public accessor for the "rollback exception" so that you can view it under pdb. chaining a traceback that's not part of the block would produce misleading and confusing tracebacks, that object is only supposed to be used to augment the string that's in the |
Beta Was this translation helpful? Give feedback.
-
|
Great, this is would have definitely saved me some time :) @zzzeek Is it alright with you if I would post a pull request with the changes proposed? |
Beta Was this translation helpful? Give feedback.
-
|
sure, give it a try |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
When presented with the Exception
InvalidRequestError("Can't operate on closed transaction inside context manager. Please complete the context manager before emitting further commands.")It could be very helpful to have a the original exception which caused the transaction to rollback in the first place, as part the chain.I'd be happy to contribute the code change, should this be proposal be accepted, as I've already experimented with locally updating the raise point at
TransactionalContext._trans_ctx_checkas part of my investigation of an issue leading me down this particular path.WDYT?
Beta Was this translation helpful? Give feedback.
All reactions