Inconsistent session state after raising exception in after_commit event

[Oleg Lomaka]

If you raise an exception in after_commit event handler, then session.is_active is False, but if you try to call session.begin(), you will get an error: "This session is in 'committed' state; no further SQL can be emitted within this transaction."

session.begin() will work only after manually calling session.close(). I am not sure is it ok to call close in this case. And not sure you consider this as actually a bug, but apparently the session is in some inconsistent state after raising exception in after_commit event handler.

@event.listens_for(session, 'after_commit')
def handler(session):
    raise Exception

try:
    with session.begin():
        pass
except Exception as e:
    pass

session.begin() # raises "This session is in 'committed' state; no further SQL can be emitted within this transaction."

[Mike Bayer]

I'm not sure what it should do. you've raised an exception when the session is attempting to complete the "committed" state. so it stopped doing that. what should it do, wrap your exception, finish everything else it was doing, then re-raise? what did your exception mean then ?

[Mike Bayer]

just an FYI I have no plans to move on this without a discussion of why someone needs to raise an exception in after_commit(), what it's expected to accomplish, and why someone would want to continue working w/ that session afterwards (and also can't call close()), what would be expected of its state at that point.

[Oleg Lomaka]

In my case, I'm not control the code that raises exception. And even if I control it, some library I use in my after_commit handler may raise something. All I need is a clear documented way how to handle it correctly.

1. Should I wrap all code in after_commit handler in try/except (my opinion, this is quite questionable way...)

2. If no, then should I call session.close() in such cases? If I should, then how to determine such situation? As I wrote, session.is_active is False. What is the way to know that session.begin() will fail and it is needed to call session.close() before?

[Mike Bayer]

OK so first, note that there are dozens of event hooks all throughout the ORM and Core, and the case for most of them is, the result of an unchecked exception thrown that cancels the handler and all the code calling it is undefined. Here, the request is to "define" the behavior for just one of those event hooks. Without additional information, it would be inconsistent for me to "define" the behavior of just this one event hook, without going through all of the event hooks everywhere and deciding what each and every one should do when exceptions are thrown. This would not only be a huge effort, it would also potentially cause problems downstream as it might mean squashing exceptions that should be raised, etc.

With the above, the point I'm hoping to make here is that I need an extremely specific information on exactly why the Session here, for this one event, should have defined behavior. I asked that I need to know "why someone needs to raise an exception in after_commit()", and so far the answer is, "some other library I'm using is doing it". OK, so, what is this exception then, is it defined by this library that this exception may be thrown ? (if so, then yes, you should handle it). Or is it a total surprise like a KeyError that should not be thrown? (if so, then this is a bug in that library, you should report it on their bug tracker, and in the meantime, work around it by catching for that exception and handling appropriately).

If, OTOH, you need this exception to actually propagate, such that "session.commit()" itself should be throwing this exception, then as I asked, I need to know what do you expect here. Why would session.commit() throw an exception after the database COMMIT has occurred, and what does that mean ?

The impression I'm getting so far is that you don't want your after_commit handler to actually be interrupting session.commit(), and thar your after_commit logic may be expected to throw exceptions. In that case then yes you need to catch that.

[Oleg Lomaka]

Thank you for detailed explanation.

I got your point that all event handlers should handle internal exceptions and do not propagate them outside. In our case that actually was a bug in our code and we have fixed it. But it was hard to understand from logs what is happening.

Now can we consider a situation from the point of view of the code that manages transactions. I have long running task that works in a loop

while True:
    try:
        with session.begin():
            pass # do the thing
    except Exception:
        log.error()
    finally:
        session.expunge_all()

And in some cases when session is broken and in undefined state (because of bug or db unavailability) I need a way to break the loop and restart. I think to rewrite it this way

class SessionError(Exception):
    pass


while True:
    try:
        try:
            tx = session.begin()
        except SQLAlchemyError as e:
            log.exception("")
            raise SessionError()
        with tx:
            pass # do the thing
    except SessionError:
        raise
    except Exception:
        log.error()
    finally:
        session.expunge_all()

From your explanation I understand it would be better to abandon the session, that is in undefined state, and start the db connection over, then call session.close().

If you consider this as accepted usage of SA session management, then this bug is not a bug. Anyway I think it should be documented that event handlers can not throw errors.

[Mike Bayer]

From your explanation I understand it would be better to abandon the session, that is in undefined state, and start the db connection over, then call session.close().

OK, so this is more of a real request then, session should be recoverable in all cases from any arbitrary exception thrown - it's not really about after_commit(). And I now see that you do want it propagated.

So of course, we could try to provide for this, that is, keeping the Session consistent no matter what occurred. The current state of Session is that it has much improved in keeping its internal state consistent within the scope of errors thrown by the database, in unusual places such as when rolling back a transaction. For the event handlers, this step hasn't been taken yet.

Looking at the source, it's not clear that this is always possible. An example is the _remove_newly_deleted method. This method fires off after a flush finishes, and the objects that were marked as DELETE are to be removed from the session as "persistent" objects:

    def _remove_newly_deleted(self, states):
        persistent_to_deleted = self.dispatch.persistent_to_deleted or None
        for state in states:
            if self._enable_transaction_accounting and self.transaction:
                self.transaction._deleted[state] = True

            if persistent_to_deleted is not None:
                # get a strong reference before we pop out of
                # self._deleted
                obj = state.obj()

            self.identity_map.safe_discard(state)
            self._deleted.pop(state, None)
            state._deleted = True
            # can't call state._detach() here, because this state
            # is still in the transaction snapshot and needs to be
            # tracked as part of that
            if persistent_to_deleted is not None:
                persistent_to_deleted(self, obj)

So here, the event is being fired off for every object within a collection, and if this loop is interrupted, the Session is now in an invalid state. Only if we caught the exception in the event and squashed it from propagating would we be able to finish.

So as far as exceptions from event handlers that are allowed to propagate outwards, I can't make a guarantee that the Session will be in a usable state subsequent to that. As far as making an event handler "safe" so that an exception in the handler is not propagated outwards, since you're decorating a function anyway, easy enough to just add another decorator that prevents exceptions from propagating.

Overall, I think relying on session.expunge_all() at the bottom is the most direct way to deal with this, just change it to a close(). This does expunge_all() (removes all object-related state) and then closes self.transaction (removes all connection-related state). I know that older SQLA docs might have said "expunge_all()" is theoretically enough but if you're at the end of a request, definitely session.close() is more appropriate.

Anyway I think it should be documented that event handlers can not throw errors.

they can, but the Session needs to be closed() afterwards. Agree every event handler throughout the whole Core/ORM should include a docstring over what's expected exception-wise. The proper resolution differs based on target object.

[Oleg Lomaka]

And I now see that you do want it propagated

Actually I do propagate exception just to stop an infinite loop. This will stop current thread and start it over with new db connection.

Overall, I think relying on session.expunge_all() at the bottom is the most direct way to deal with this, just change it to a close()

Good point. Thank you. Earlier I delegated session closing to session.begin() context manager. And neglected the case when event handler may raise an exception and leave session in half-opened state.

Should I close this ticket? Or you are going to do something in context of it? It starts to look more like mailing list thread.

[Mike Bayer]

there is a longer term goal to add documentation to all event handlers as far as what the behavior is with exceptions - a small number of event hooks do actually support exception being thrown as part of their public API, while most others, the behavior is undefined. But I'd close the ticket here, sure.

[Mike Bayer]

this one event hook and lots of others could be "fixed" so that the state of the session is consistent, but for others, it's not possible. would rather not try to make this guarantee.

[Mike Bayer]

• add session.close() w/ rationale to top-level "using transactions" section. References #3974

Change-Id: Idb650cbe9825cfae893ee917132b5b9d693f0c6c

→ <<cset 7fc7492d86f6>>

[Mike Bayer]

• add session.close() w/ rationale to top-level "using transactions" section. References #3974

Change-Id: Idb650cbe9825cfae893ee917132b5b9d693f0c6c (cherry picked from commit 7fc7492d86f6e5ca105743a184cd07190e9f9b28)

→ <<cset f830fff2583b>>