Unsafe transactions

[vercotux]

I've reviewed your code a bit and noticed you do not use any database transactions when inserting a new Transaction into the database. This is very dangerous as it can lead to dirty reads, non-repeatable reads or even phantom reads which will break your entire transaction chain by causing the balance_before and balance_after to end up with incorrect values!

You need to read the current balance from your purse and then store the Transaction both inside a database transaction! Not only that, but it has to be specifically a SERIALIZABLE transaction. Databases typically use "repeatable read" transactions by default which are not safe from phantom reads. Unfortunately though while Yii2 does have built-in ActiveRecord transactions it does not let us set the isolation level in advance, so for now you may need to override the insert() method. :(

[lynicidn]

which will break your entire transaction chain by causing the balance_before and balance_after to end up with incorrect values!

select record for transaction this is only one solution for https://en.wikipedia.org/wiki/Race_condition#Computer_security

u can solve it via optimistick lock

[lynicidn]

transaction is not dangerous. Dangerous code which write coders ^^

[vercotux]

Indeed, the code is dangerous, because it does not use transactions. Optimistic locking is not enough. It does not solve this problem. Only a serializable transaction can prevent things like phantom reads.

[lynicidn]

what is phantom read? u can lock record on read where u select need mark for update or denied update record via optimistick or other lock http://stackoverflow.com/questions/795237/mysql-race-conditions#795237

[vercotux]

Why doesn't this library do this? Why do I have to do it? It is the library's job to ensure basic transactional isolation of its own transactions.

[lynicidn]

Two-phase locking is the most common transaction concurrency control method in DBMSs, used to provide both serializability and recoverability for correctness. In order to access a database object a transaction first needs to acquire a lock for this object. Depending on the access operation type (e.g., reading or writing an object) and on the lock type, acquiring the lock may be blocked and postponed, if another transaction is holding a lock for that object.

(c) https://en.wikipedia.org/wiki/Isolation_%28database_systems%29

[omnilight]

@vercotux @lynicidn thank you for your participation. Could you please create pull request or some gist for improvements over transaction usage?

[vercotux]

For starters you can add this to the Transaction model:

public function transactions() {
    return [
        self::SCENARIO_DEFAULT => self::OP_ALL,
    ];
}

It won't solve the problem completely (because of Yii2 bug linked in my initial post), but it will greatly improve the safety. In order to workaround the Yii2 bug you can override (at least) the insert() method like so:

public function insert($runValidation = true, $attributes = null) {
    if ($runValidation && !$this->validate($attributes)) {
        Yii::info('Model not inserted due to validation error.', __METHOD__);
        return false;
    }
    $transaction = static::getDb()->beginTransaction(\yii\db\Transaction::SERIALIZABLE);
    try {
        $result = $this->insertInternal($attributes);
        if ($result === false) {
            $transaction->rollBack();
        } else {
            $transaction->commit();
        }
        return $result;
    } catch (\Exception $e) {
        $transaction->rollBack();
        throw $e;
    }
}

[lynicidn]

You can call [[setIsolationLevel()]] after the transaction has started.

[lynicidn]

beforeSave() { $this->db->setIsolationLevel(); return parent::beforeSave(); }

[vercotux]

That will not work with MySQL.

[lynicidn]

try:

public function insert($runValidation = true, $attributes = null) {
    $this->db->schema->setTransactionIsolationLevel(); 
    return parent::insert($runValidation, $attributes)
}

[vercotux]

Have you tried it? It won't work either, because you're setting the isolation level before the transaction has begun. See the documentation:

Sets the isolation level of the current transaction.

Like I said, the only way to properly resolve this without workarounds is to fix the issue in Yii2 first.