A retried request is processed outside of Tx when the commit failure object log entry failed to commit

[jbrichau]

In the GRGemStonePlatform>>seasideProcessRequestWithRetry:resultBlock: method, when the addition of the 'commit failure' object log entry (see below) fails to commit, it leaves the process in Tx state.

self 
    saveLogEntry: (WAObjectLogEntry warn: 'Commit failure - retrying' request: aNativeRequest url object: conflicts)  
    shouldCommit: true.

When the request is retried (or when another waiting request enters the Tx-mutex protected block), the vm is in transaction. Next, the first statement of the Tx-block makes it leave transaction mode.

        System inTransaction
            ifTrue:[ self doAbortTransaction. ]
            ifFalse: [ self doBeginTransaction ].

Because we are in manual transaction mode, I think this should be:

        System inTransaction
            ifTrue:[ self doAbortTransaction. self doBeginTransaction. ]
            ifFalse: [ self doBeginTransaction ].

Though a failure to commit on the object is unlikely, is it impossible?

[jbrichau]

I only simulated failed commits on the object log, thereby exposing this behavior. The result of processing the request out-of-transaction is that the Seaside request's state changes are non-persistent: the request answers correctly but the Seaside session state is lost, leading to strange behavior of the Seaside app.

[dalehenrich]

@jbrichau, the code was written assuming that when the block started the system would be out of transaction if it was in manual transaction mode ... perhaps an error should be thrown there instead, because being in transaction is indicative of a deeper problem ... I wouldn't argue that your suggestion is not good, but it may mask some deeper errors ... like now ...

I think that the problem must be that we expect saveLogEntry:shouldCommit: to do a commit ... if it doesn't commit, then you are left in transaction and things go haywire ... so the commit failure logic is probably the right place to put the bugfix and I think an error might be called for in the case that we're in transaction and in manual transaction mode at the top of the loop ... so can prevent the kind of mess the seaside session state can get into when a commit is needed ... as you've observed ...

Am I making sense?

I think that all senders of of #doTransaction: need to check the return value and take appropriate action to restore the transaction state ... or better yet (yikes) another error if a commit failure does occur?

Am I going off the deep end?

[jbrichau]

@dalehenrich nope, I'm not falling off a cliff yet :)

I agree it's better to make sure we do not re-enter the loop in transaction. But it's strange that when it would enter the loop already in a transaction, that it's aborting the transaction and not starting a new one (like now). Instead of throwing an error, I would also log an error and keep serving requests.

So I'm thinking the fix should:

• make sure all exit points from the request handling method are outside of a tx
• make sure that a request is always processed in a tx
• log an error when we come in the request handling already in tx

I'll get a pull request whipped up soon.

[dalehenrich]

Keep in mind that the inTransaction logic is assuming that we must be in automatic transaction mode ... if we are in manual transaction mode then a beginTransaction is called for ... so if we are going to bullet proof/booby trap the entry to this block, then we should also check which mode we are in ....

automatic transaction mode is used when you are running seaside in a development vm ...