Skip to content
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

Closed
nsossonko opened this issue Mar 16, 2016 · 17 comments
Closed

Issue with transaction manager, doesn't manage transactions correctly #11554

nsossonko opened this issue Mar 16, 2016 · 17 comments
Milestone

Comments

@nsossonko
Copy link
Contributor

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.

@nask0
Copy link

nask0 commented Apr 12, 2016

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
Phalcon Version : 2.0.10

Is there are known workaround for this or i must figure it out myself ? :)
Thanks in advance.

@nsossonko
Copy link
Contributor Author

I think it's just because you never started the transaction. You have to call begin if you don't pass true for autoStart...

sergeyklay added a commit that referenced this issue Apr 12, 2016
Fixes #11554 - Transaction Manager transaction accounting
@sergeyklay
Copy link
Contributor

@nsossonko @nask0 Could you please check now 2.1.x branch?

@nask0
Copy link

nask0 commented Apr 12, 2016

@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.

@nask0
Copy link

nask0 commented Apr 12, 2016

@sergeyklay i still get the same exception ( branch 2.1.x install log / exception dump ).
However, @nsossonko was right and adding $tx->begin() after getting transaction fixes the issue, but i wonder what is autoBegin parameter for then?

@sergeyklay
Copy link
Contributor

Try zephir build from project root

@nask0
Copy link

nask0 commented Apr 12, 2016

@sergeyklay sorry, i got zephir error, maybe it's something on my side, but currently do not have time to investigate further.
If you have any tips, I will be glad to help :)

@sergeyklay
Copy link
Contributor

@nask0 Could you please use latest Zephir? (master branch)

@nask0
Copy link

nask0 commented Apr 12, 2016

I am already with it:

[nask0@h0m3 zephir]$ git branch
* master
[nask0@h0m3 zephir]$ git pull
Already up-to-date.
[nask0@h0m3 zephir]$

More debug info:

[nask0@h0m3 zephir]$ php -v 
PHP 5.6.20 (cli) (built: Mar 31 2016 06:11:29) 
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
    with Xdebug v2.3.3, Copyright (c) 2002-2015, by Derick Rethans
[nask0@h0m3 zephir]$ phpize -v
Configuring for:
PHP Api Version:         20131106
Zend Module Api No:      20131226
Zend Extension Api No:   220131226
[nask0@h0m3 zephir]$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/5.3.1/lto-wrapper
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --disable-libgcj --with-isl --enable-libmpx --enable-gnu-indirect-function --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 5.3.1 20151207 (Red Hat 5.3.1-2) (GCC) 
[nask0@h0m3 zephir]$ httpd -v
Server version: Apache/2.4.18 (Fedora)
Server built:   Jan  4 2016 08:15:18
[nask0@h0m3 zephir]$ 

Here is Phalcon build info before trying zephir build

Phalcon Version 2.1.0r
Build Date Apr 12 2016 21:04:48
Powered by Zephir   Version 0.9.2a-dev

@sergeyklay
Copy link
Contributor

@steffengy Can you please figure out what's going on with Zephir on Fedora?

parser/parser/scanner.c:30:14: error: expected identifier or ‘(’ before ‘extension
extern char *strndup(const char *s, size_t len);

@nask0
Copy link

nask0 commented Apr 14, 2016

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" :)
Also i was able to find pattern what works when :

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()

@nsossonko
Copy link
Contributor Author

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.

@daison12006013
Copy link

daison12006013 commented Apr 14, 2016

@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.

@nask0
Copy link

nask0 commented Apr 15, 2016

@nsossonko thanks for comment. I intetionally menation "pseudo" code in my prev comment.
However, lets leave behind for a moment the fact that the default value of autoBegin in transaction manager is (boolean) true and strart from very beginning :

  1. Here is how i load application
  2. This is DataService class that just load data services
  3. This is DataServiceAbstract (note _getTransaction() method)
  4. Here is actual Data Services that perform transactions : User, ApiClients, Verifications
  5. Finally in controller, transactions functions get called in the following order:
$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.

@nsossonko
Copy link
Contributor Author

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.

@nask0
Copy link

nask0 commented Apr 15, 2016

@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.
Thanks all again and keep up the good work, Phalcon is awesome project (for ten long years in PHP this is the only framework i still use today)

@sergeyklay
Copy link
Contributor

I'll open this issue again if the problem is actual

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants