-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue with transaction manager, doesn't manage transactions correctly #11554
Comments
I guess this will fix the bug i encountered and no need to open a separate issue ? Bug is : // Users Data Service class
public function save( array $data )
{
try {
// requested data is filtered and validated at this point, start db transaction
$tx = $this->_getTransaction(); // returns TransactionMager->get() getted from DI
$newUser = new UsersModel();
$newUser->setTransaction( $tx );
$newUser->setName( $data['name'] );
// more setters for Users model
if( false === $newUser->save() ) {
$tx->rollback( 'Unable to save new user' );
}
$newDevice = new Devices();
$newDevice->setTransaction( $tx );
// more setters for Devices model
if( false === $newDevice->save() ) {
$tx->rollback( 'Unable to save new device' );
}
$tx->commit();
return true;
} catch( \Exception $e ) {
$this->_setValidationError( 'unexpected', 'Unable to register user : ' . $e->getMessage() );
return false;
}
}
// SMS Data Service class
public function saveVerificationSMS( $verificationCode, $smsData )
{
try {
$tx = $this->_getTransaction();
$message = new SmsMessagesSentModel();
$message->setTransaction( $tx );
// more setters for SmsMessagesModel()
if( false === $message->save() ) {
$tx->rollback( 'Unable to save SMS message.' );
}
$verification = new VerificationModel();
$verification->setTransaction( $tx );
// more setters for VerificationModel
$verification->setCreatedAt( Date::getUTCTimestamp() );
$verification->setUpdatedAt( Date::getUTCTimestamp() );
if( false === $verification->save() ) {
$tx->rollback( 'Unable to save new verification.' );
}
$tx->commit();
return true;
} catch( \Exception $e ) {
$this->_setValidationError( 'unexpected', 'Unable to save verification SMS (' . $e->getMessage() . ')' );
return false;
}
}
// In controller :
$userDataService->save( $data ); // everything works fine
// throws PDO exception : "There is no active transaction", BUT all records are saved properly
$smsDataService->saveVerificationSMS( 'blah', [] ); PHP Version: 5.6.20 Is there are known workaround for this or i must figure it out myself ? :) |
I think it's just because you never started the transaction. You have to call begin if you don't pass true for autoStart... |
Fixes #11554 - Transaction Manager transaction accounting
@nsossonko @nask0 Could you please check now |
@nsossonko The documentation never told me to do begin() (also there is a autoBegin parameter passed to TManager->get() ), moreover transaction is saved successfully, just always raises a exception. @sergeyklay Ok, I will try 2.1 branch, thanks. |
@sergeyklay i still get the same exception ( branch 2.1.x install log / exception dump ). |
Try |
@sergeyklay sorry, i got zephir error, maybe it's something on my side, but currently do not have time to investigate further. |
@nask0 Could you please use latest Zephir? (master branch) |
I am already with it:
More debug info:
Here is Phalcon build info before trying zephir build
|
@steffengy Can you please figure out what's going on with Zephir on Fedora?
|
I found interesting that if i put $tx->begin() on the very first transaction that is get executed (from TxManager), none of transactions is saved (still model have generated id and so on i.e. model->save() returns != false), but if i remove just this first begin, everything "works" :) The following flow (without begin) works only for first executed transactions (that created > 1 new models !) and in all subsequent transactions that creates and saves only one new model $tx = $this->tansactionManger->get();
$model = new Model()
if ( false === $model->save() ) {
$tx->rollback( 'Unable to save API Client' );
}
$tx->commit() The flow with begin() $tx = $this->tansactionManger->get();
$tx->begin();
$model = new Model()
if ( false === $model->save() ) {
$tx->rollback( 'Unable to save Model' );
}
$model2 = new Model2()
if ( false === $model2->save() ) {
$tx->rollback( 'Unable to save Model 2' );
}
$tx->commit(); works on all transactions after the first but only when saving multiple models. Here is the pseudo of what my code look like and honestly i am not happy with consistency :) : // transaction 1 (transactionManager is shared in DI and so on)
$tx = this->transactionMager->get()
// creates 4 new models, then :
if ( ! model->save() ) { $tx->rollback(); }
if ( ! model2->save() ) { $tx->rollback(); }
// ...
$tx->commit()
// transaction 2 ( create and saves only one new model), do not work with begin();
$tx = this->transactionMager->get()
// creates 1 new models, then :
if ( ! model->save() ) { $tx->rollback(); }
// ...
$tx->commit()
// transaction 3, (create and saves 3 new models)
$tx = this->transactionMager->get()
$tx->begin(); // do not works without begin
if ( ! model->save() ) { $tx->rollback(); }
if ( ! model2->save() ) { $tx->rollback(); }
if ( ! model3->save() ) { $tx->rollback(); }
// ...
$tx->commit() |
The way transactions work is if you don't call begin on the transaction (or pass true for autoStart to get()) it will just create the model without a transaction. So what you are saying is not surprising. What you should test to verify this is: if you don't call begin and a second model fails after the first was created, then call rollback, see what happens. You should see the first model is created and the second is not. Also, in your recent code you posted, it doesn't show that you are attaching the transaction to each model, which is necessary. You must call setTransaction on each model with the transaction. |
@nsossonko I don't know if this solution works on your side too, try to transform your 'db' service as shared/singleton. I initialized the db di first before dispatcher di, which causes the error on my side. |
@nsossonko thanks for comment. I intetionally menation "pseudo" code in my prev comment.
$dataServiceUsers->save();
$dataServiceApiClients->register();
$dataServiceVefications->save() Now, if i don't miss anything, according to Phalcon documentation for isolated transactions - the code should work, but it only works if i make some weird things like setting $tx->begin() in some of transactions as i already said. |
Actually @daison12006013 might be correct, you're not setting the db and transactionManager via setShared so it may be creating more than one. About autoBegin, you seem to be right about that, so it shouldn't be required. I use the transaction manager extensively in my application and it works quite well for me. |
@nsossonko ok, thanks. I will check all my code again and play with DI once i am done my current project (deadline is soo soon :/). I will let you know guys, if i find anything else. |
I'll open this issue again if the problem is actual |
There's an issue with the transaction manager that makes it work very poorly. I will upload a pr that fixes it, but, as of now, I don't have time to run tests really on it. I have extended the manager and overriden this function so that it works properly for me, so that much testing I have done.
The text was updated successfully, but these errors were encountered: