Problem

Rendering the cartBlock was moved to a lazy builder in 3.x. This functions fine until another you introduce another lazy builder that also calls \Drupal\commerce_cart\CartProviderInterface::getCarts(). The first call of getCarts() returns no carts. The second call to run returns carts as expected.

Any code that doesn't use lazy builders and calls getCarts() on the page will work as expected, and it will unbreak all of the the lazy builders that call getCarts(). Examples would be the cart page and checkout.

I encountered this because I have two cart blocks on my page. One renders the button. The other block renders the cart block view as an offcanvas element. They will both properly render if both blocks have the exact same settings (dropdown, show when empty).
I thought maybe there was a caching issue so I copied the block and lazy builder into a custom module and modified it so they would be completely separate from each other. That did not work. The bug still triggers.

I do not know how to fix this.

How to reproduce

The quick and easy way to reproduce this is to place two cart blocks on a site, and set one to show a dropdown and the other to not show a dropdown. The block placed higher up the page will render with the expected count and dropdown contents. The one placed below it will show an empty cart.

Now go to the cart page or checkout and you will see both cartblocks will render properly.

Issue fork commerce-3578096

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

rhovland created an issue. See original summary.

rhovland’s picture

rhovland
he/him
Primary language English
Location Oregon
commented

I examined the code of cartProvider in a step debugger as it generated two cart blocks via the lazy builder and it seems like they are both being executed asynchronously at the same time and what ends up happening is that $this->cartData[$uid] gets set, but the contents are empty for one of the threads so instead of doing a query for both it's returning an empty result.

If I add $this->cartProvider->clearCaches() to the cartBlock function in the lazy builder the issue goes away.

  public function cartBlock(bool $dropdown, bool $show_if_empty): array {
    $this->cartProvider->clearCaches();

    $cacheable_metadata = new CacheableMetadata();
    $cacheable_metadata->addCacheContexts(['user', 'session', 'cart']);
    $cart_cache_tags = [];

    $carts = $this->cartProvider->getCarts() ?? [];

So if I understand the issue correctly this is a race condition due to the cartProvider sharing a cache with two simultaneously running lazy builders.

If the cache has already been generated by the initial page load (cart page) then it returns a complete cached result and everything functions as it should.

tbkot made their first commit to this issue’s fork.

tbkot opened merge request !645

rhovland’s picture

rhovland
he/him
Primary language English
Location Oregon
commented

Can confirm that the MR fixes the issue and if there is already a cache from another invocation of cartProvider it uses that for the lazy builders.

tomtech’s picture

tomtech commented

@tbkot,

While this likely fixes it for the most common case of a singular cart, this would still cause a race condition if there are multiple carts for a user.

For example, if there are 2 carts:

1. Thread one starts and sees no value set for the given uid, so fetches the cart ids to consider and begins looping.
2. Thread one adds the first cart to the array.
3. Thread two starts and see that there is a value set for the given uid, and immediately returns, with an array containing one cart.
4. Thread one continues the next iteration, setting a second value to the array, and returning it.

The first thread will return two carts.

The second thread will return the first cart only.

We should likely:
1. Build the value in a local variable, and set it once at the end, rather than iteratively updating the array. This would prevent a second process from picking up a partial array.
2. Consider adding a lock. If we don't then, both process will perform the same fetch and build. At least in that case the values should be the same...just wasted processing time performing the same task. If we add a lock, then we should check again if the value is set, so we can short circuit if another process built it while we requested the lock.

e.g. something like:

$lock = $this->requestLock($uid);
// Double check that the value hasn't been set while waiting for the lock.
// If we are the first to get a lock, then it won't be set.
// If we are NOT the first, then the first likely already built the value, and we can just return it.
if (isset($this->cartData[$uid] )) {
$this->releaseLock($uid);
return $this->cartData[$uid];
}
// build the data using a local variable, rather than iteratively populating the array.
$cart_data = ...;

$this->cartData[$uid] = $cart_data;
$this->releaseLock($uid);
return $cart_data;

pnigro’s picture

pnigro commented
Status: Active » Needs review

Thank you for MR !645. It fixes my issue #3577394: Multiple carts are created when adding different variations of the same product to the shopping cart.

jsacksick’s picture

jsacksick commented

Tested this change on a project where the fix from #3573990: Issue with static cache related to fibers and cart loading in Drupal 11.3+ was causing issues and everything seems to be working as expected so I'm willing to merge this.

@tomtech: Using the lock for this is probably overkill, at least for now... Let's revisit this later if needed, hoping it won't be :).

  • jsacksick committed acdad08d on 3.x authored by tbkot 
    fix: #3578096 cartProvider returns no carts if used by multiple lazy...
jsacksick’s picture

jsacksick commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

codebymikey’s picture

codebymikey commented

This issue also affects the supposedly stable 3.1.0 and 3.2.0 releases, and ideally a patch release should be backported addressing them too as currently certain products are causing the site to have multiple carts created.

Or marking them as unsupported and that 3.3.x should be the version to move onto.

tbkot’s picture

tbkot commented

@codebymikey, the issue is not caused by Commerce but rather by Drupal 11.3, where Fibers is used for rendering. So all previous Commerce 3.x will work properly with Drupal 11.2 and lower.

codebymikey’s picture

codebymikey commented

Yeah I know, I just mentioned the 3.1 and 3.2 releases because they're meant to also support 11.3, and were the ones I'd updated to, so thought it was worth either patching or removing them as recommended releases (which I can see has already been done, so thanks for that!).