bavix / laravel-wallet

It's easy to work with a virtual wallet
https://bavix.github.io/laravel-wallet/
MIT License
1.14k stars 230 forks source link

Multiple Redis balance refresh after manual DB transaction rollback #347

Closed iamroi closed 3 years ago

iamroi commented 3 years ago

Hey @rez1dent3 :)

This is closely related to #258

I have a DB transaction that transfers amounts in and out of at least 100+ users wallets

as a sample scenario,

DB::beginTransaction();

forEach($wallets as $wallet) {
    $wallet->deposit(100);
}

// something happened in business logic so rolling back
DB::rollBack();

// at this point Redis `balance` for all/some $wallets are incorrect

// to fix that I will have to write a loop for 100+ wallets and refresh balance which is a really expensive operation
forEach($wallets as $wallet) {
    $wallet->refreshBalance();
}

Is there a way I can avoid the refreshBalance for all wallets? Maybe I could just delete the Redis balance caches for all the wallets affected?

Any help would be appreciated! Thanks

rez1dent3 commented 3 years ago

@iamroi Hi. You have an interesting enough case and it will not be possible to solve it using the laravel wallet.

I have a question, why is it rolled back later according to business logic? Maybe it is worth charging only in case of success with the help of queues? https://laravel.com/docs/8.x/queues

It seems to me that you need to write your own store-provider to write and add feature "force push to redis". Example: https://github.com/bavix/laravel-wallet/blob/master/src/Simple/Store.php#L9

iamroi commented 3 years ago

Hi @rez1dent3 Thanks for your response!

You are right and the whole process is already happening on a queue. The process is just about transferring amounts between wallets based on bunch of formulas.

Also want to note that in my case there will be multiple instances of this same queue processes running in parallel possibly involving same wallets.

I don't think there will be an issue most of the time but worried about unexpected cases for eg. database connection etc during the process.

I ended up using the below code and it seems to be working with my tests. But I'm not sure if this is safe to use and don't cause any race condition problems.

Could you please let me know if you see any issues with this approach?

// unexpected error occurred so rolling back
DB::rollBack();

// for example
$wallets = getAllWalletsInvolvedInTheProcess();

app(LockService::class)->lock($this, __FUNCTION__, static function () use($wallets) {
    foreach ($wallets as $wallet) {
        app(Store::class)->taggedCache()->forget(
            app(StoreService::class)->getCacheKey($wallet)
        );
    }
});

Thanks for your time.

rez1dent3 commented 3 years ago

race condition will be. At the moment, until your refresh passes, the old balance will live and the user can take advantage of it

iamroi commented 3 years ago

Thanks @rez1dent3 that makes sense!

I guess I will have to use the lockForUpdate - https://laravel.com/docs/8.x/queries#pessimistic-locking wherever I update the balance to be safer.

Appreciate your help :)