fix(medusa): ensure transaction is reused when service calls itself #6089

srindom · 2024-01-15T10:40:10Z

What

ProductService.create calls ProductService.retrieve before returning. This fix ensures that the manager created in the atomicPhase is used when ProductService.retrieve is called.

Why

Without the explicit use of the same manager in the retrieve call, race conditions easily occur if you have concurrent ProductService.create calls. The code below can reproduce the scenario:

  const productData = [
    {
      barcode: `1234233423-${Math.random() * 100}`,
      external_id: `1234233423-${Math.random() * 100}`,
      description: "super cool product",
      discountable: true,
      hs_code: "1234213",
      origin_country: "DK",
      title: `Super cool product ${Math.random() * 100}`,
      type: { value: "Eyewear" },
      sales_channels: [{ id: sc.id }],
    },
    {
      barcode: `1234233423-${Math.random() * 100}`,
      external_id: `random-external-id-${Math.random() * 100}`,
      description: "super cool product",
      discountable: true,
      hs_code: "1234213",
      origin_country: "DK",
      title: `Super cool product ${Math.random() * 100}`,
      type: { value: "Eyewear" },
      sales_channels: [{ id: sc.id }],
    },
  ];

  const result = await Promise.all(
    productData.map(async (product) => productService.create(product))
  );

Explaination

What happens is the following:

Request 1 calls ProductService.create. This in turn calls atomicPhase which sets the ProductService.transactionManager_ = txForReqOne
Request 1 creates the product in the DB with txForReqOne.
Request 2 calls ProductService.create. This in turn calls atomicPhase which sets the ProductService.transactionManager_ = txForReqTwo
Request 1 reaches the end of ProductService.create where this.retrieve is called. Because the ProductService is a singleton the retrieve call will attempt to use txForReqTwo to fetch the product.
Error: since txForReqTwo can't read the data of txForReqOne, the product is not found.

changeset-bot · 2024-01-15T10:40:13Z

⚠️ No Changeset found

Latest commit: 1c569a4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2024-01-15T10:40:18Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
medusa-dashboard	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 19, 2024 10:42am

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
api-reference	⬜️ Ignored (Inspect)	Visit Preview	Jan 19, 2024 10:42am
docs-ui	⬜️ Ignored (Inspect)	Visit Preview	Jan 19, 2024 10:42am
medusa-docs	⬜️ Ignored (Inspect)	Visit Preview	Jan 19, 2024 10:42am

adrien2p · 2024-01-15T13:37:52Z

Nice catch 💪 you found that probably because there is somewhere where the service is not called within a transaction right?

olivermrbl · 2024-01-15T13:48:20Z

Should we fix this across the entire service? Think we use this pattern in a couple of other places

adrien2p · 2024-01-15T13:49:30Z

Should we fix this across the entire service? Think we use this pattern in a couple of other places

Is it also the only service or should we check them all?

olivermrbl · 2024-01-17T12:35:31Z

@adrien2p, I will migrate the rest of the ProductService before we merge, but will keep the rest (if any) for a separate PR

adrien2p · 2024-01-17T12:37:52Z

Sounds good to me, if the pr is not done just after we might need to create a ticket?

srindom · 2024-01-18T09:34:32Z

@olivermrbl, @adrien2p - can we apply automerge or is there still more here?

adrien2p · 2024-01-18T09:36:30Z

I ll do a final review, haven't seen the last commit. Butnif you want we can auto merge and if i have any comments we can handle them separately to not block it

adrien2p

LGTM 🔥 couple of comments

adrien2p · 2024-01-18T09:46:18Z

packages/medusa/src/services/product.ts

@@ -594,7 +595,7 @@ class ProductService extends TransactionBaseService {
          )
      }

-      const result = await this.retrieve(product.id, {
+      const result = await this.withTransaction(manager).retrieve(product.id, {


todo: line 559 is missing. maybe we can do something like productServiceTx = this.withTransaction(manager) and then use it in both places?

@srindom I ve seen that you have merged but there is still a missing transaction here

adrien2p · 2024-01-18T09:47:51Z

packages/medusa/src/services/product.ts

@@ -833,7 +834,7 @@ class ProductService extends TransactionBaseService {
        )
      }

-      const result = await this.retrieve(productId)
+      const result = await this.withTransaction(manager).retrieve(productId)


suggestion: same as the previous one to not recreate the instance multiple times everytime

fix: ensure transaction is reused when service calls itself

846dad3

srindom requested a review from a team as a code owner January 15, 2024 10:40

Merge branch 'develop' into fix/create-product-transaction-management

c56bde9

vercel bot deployed to Preview – medusa-dashboard January 15, 2024 11:13 View deployment

olivermrbl changed the title ~~fix: ensure transaction is reused when service calls itself~~ fix(medusa): ensure transaction is reused when service calls itself Jan 15, 2024

Merge branch 'develop' into fix/create-product-transaction-management

9690ae5

srindom requested a review from a team as a code owner January 16, 2024 16:47

vercel bot deployed to Preview – medusa-dashboard January 16, 2024 16:49 View deployment

riqwan approved these changes Jan 17, 2024

View reviewed changes

olivermrbl and others added 2 commits January 17, 2024 13:39

migrate rest of ProductService

14f7dcd

Merge branch 'develop' into fix/create-product-transaction-management

23f5396

vercel bot deployed to Preview – medusa-dashboard January 17, 2024 12:43 View deployment

Merge branch 'develop' into fix/create-product-transaction-management

38a78ac

vercel bot deployed to Preview – medusa-dashboard January 18, 2024 09:36 View deployment

adrien2p approved these changes Jan 18, 2024

View reviewed changes

Merge branch 'develop' into fix/create-product-transaction-management

1c569a4

srindom added the automerge label Jan 19, 2024

vercel bot deployed to Preview – medusa-dashboard January 19, 2024 10:42 View deployment

kodiakhq bot merged commit 1b99c5d into develop Jan 19, 2024
16 checks passed

kodiakhq bot deleted the fix/create-product-transaction-management branch January 19, 2024 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(medusa): ensure transaction is reused when service calls itself #6089

fix(medusa): ensure transaction is reused when service calls itself #6089

srindom commented Jan 15, 2024 •

edited

Loading

changeset-bot bot commented Jan 15, 2024 •

edited

Loading

vercel bot commented Jan 15, 2024 •

edited

Loading

adrien2p commented Jan 15, 2024

olivermrbl commented Jan 15, 2024

adrien2p commented Jan 15, 2024

olivermrbl commented Jan 17, 2024

adrien2p commented Jan 17, 2024

srindom commented Jan 18, 2024

adrien2p commented Jan 18, 2024

adrien2p left a comment

adrien2p Jan 18, 2024 •

edited

Loading

adrien2p Jan 19, 2024

adrien2p Jan 18, 2024

fix(medusa): ensure transaction is reused when service calls itself #6089

fix(medusa): ensure transaction is reused when service calls itself #6089

Conversation

srindom commented Jan 15, 2024 • edited Loading

changeset-bot bot commented Jan 15, 2024 • edited Loading

⚠️ No Changeset found

vercel bot commented Jan 15, 2024 • edited Loading

adrien2p commented Jan 15, 2024

olivermrbl commented Jan 15, 2024

adrien2p commented Jan 15, 2024

olivermrbl commented Jan 17, 2024

adrien2p commented Jan 17, 2024

srindom commented Jan 18, 2024

adrien2p commented Jan 18, 2024

adrien2p left a comment

Choose a reason for hiding this comment

adrien2p Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

adrien2p Jan 19, 2024

Choose a reason for hiding this comment

adrien2p Jan 18, 2024

Choose a reason for hiding this comment

srindom commented Jan 15, 2024 •

edited

Loading

changeset-bot bot commented Jan 15, 2024 •

edited

Loading

vercel bot commented Jan 15, 2024 •

edited

Loading

adrien2p Jan 18, 2024 •

edited

Loading