Skip to content

Conversation

@tw2066
Copy link
Contributor

@tw2066 tw2066 commented Oct 22, 2025

当调用enableConcurrent方法后

concurrent的值一直为 true

@PandaLIU-1111 PandaLIU-1111 requested a review from Copilot October 22, 2025 06:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a singleton-related bug where concurrent and orders properties persisted across multiple Saga instances. The solution replaces instance properties with Context-based storage, ensuring proper isolation between Saga transactions.

Key Changes:

  • Replaced instance properties $concurrent and $orders with Context-based storage using class constants as keys
  • Added initialization of concurrent and orders values in the init() method to ensure clean state for each transaction

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +37 to +38
Context::set(static::CONCURRENT, false);
Context::set(static::ORDERS, []);
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential race condition in singleton usage: if multiple threads call init() concurrently, the Context state could be overwritten unexpectedly. Consider using a transaction-specific context key (e.g., including $gid in the key) to ensure isolation between concurrent Saga instances.

Copilot uses AI. Check for mistakes.
class Saga extends AbstractTransaction
{
protected array $orders = [];
protected const CONCURRENT = self::class . '.concurrent';
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using self::class instead of static::class in the constant definition may cause issues in inheritance scenarios. If a subclass of Saga uses these constants, they should resolve to the subclass name. Consider using a runtime method to generate context keys with static::class or document that these constants should not be inherited.

Copilot uses AI. Check for mistakes.
Comment on lines +66 to +68
$orders = Context::get(static::ORDERS, []);
$orders[$branch] = $preBranches;
Context::set(static::ORDERS, $orders);
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get-modify-set pattern for updating the orders array creates unnecessary overhead. Each call retrieves the entire array, modifies it, and stores it back. If addBranchOrder is called frequently, consider whether the Context API supports direct array modification or accumulate changes locally before final persistence.

Copilot uses AI. Check for mistakes.
@tw2066
Copy link
Contributor Author

tw2066 commented Oct 22, 2025

@PandaLIU-1111 麻烦看一下这个pr

在例子中
调用该方法后

    #[RequestMapping(path: 'concurrentCase')]
    public function concurrentCase(Saga $saga): string
    {
        $this->initAccountAmount(100);

        $payload = $this->buildPayload(50);
        $saga->init();
        $saga->add($this->serviceUri . '/saga/redis/transOut', $this->serviceUri . '/saga/redis/transOutCompensate', $payload);
        $saga->add($this->serviceUri . '/saga/redis/transOut', $this->serviceUri . '/saga/redis/transOutCompensate', $payload);
        $saga->add($this->serviceUri . '/saga/redis/transIn', $this->serviceUri . '/saga/redis/transInCompensate', $payload);
        $saga->add($this->serviceUri . '/saga/redis/transIn', $this->serviceUri . '/saga/redis/transInCompensate', $payload);
        $saga->enableConcurrent();
        $saga->addBranchOrder(2, [0, 1]);
        $saga->addBranchOrder(3, [0, 1]);
        $saga->submit();
        return 'Submitted';
    }

Saga 对象中 concurrent和orders 一直存在, 不释放; 会影响 其他方法调用

@PandaLIU-1111
Copy link
Member

@PandaLIU-1111 麻烦看一下这个pr

在例子中 调用该方法后

    #[RequestMapping(path: 'concurrentCase')]
    public function concurrentCase(Saga $saga): string
    {
        $this->initAccountAmount(100);

        $payload = $this->buildPayload(50);
        $saga->init();
        $saga->add($this->serviceUri . '/saga/redis/transOut', $this->serviceUri . '/saga/redis/transOutCompensate', $payload);
        $saga->add($this->serviceUri . '/saga/redis/transOut', $this->serviceUri . '/saga/redis/transOutCompensate', $payload);
        $saga->add($this->serviceUri . '/saga/redis/transIn', $this->serviceUri . '/saga/redis/transInCompensate', $payload);
        $saga->add($this->serviceUri . '/saga/redis/transIn', $this->serviceUri . '/saga/redis/transInCompensate', $payload);
        $saga->enableConcurrent();
        $saga->addBranchOrder(2, [0, 1]);
        $saga->addBranchOrder(3, [0, 1]);
        $saga->submit();
        return 'Submitted';
    }

Saga 对象中 concurrent和orders 一直存在, 不释放; 会影响 其他方法调用

你这种用法是有点问题的,理论上你应该要用 new Saga() 的方式来调用的;

@tw2066
Copy link
Contributor Author

tw2066 commented Oct 22, 2025

@PandaLIU-1111
Copy link
Member

参考例子写法 https://github.com/dtm-php/dtm-sample/blob/master/app/Controller/SagaRedisController.php

能用单例是不是更好

这里不建议使用单例,给的用例有点问题,你这么想这个问题,假设如果我一个协程内同时存在两个 Saga 是否就出问题了
eg:

 public function complexOperation(Saga $saga1, Saga $saga2)
{

}

@tw2066
Copy link
Contributor Author

tw2066 commented Oct 22, 2025

参考例子写法 https://github.com/dtm-php/dtm-sample/blob/master/app/Controller/SagaRedisController.php
能用单例是不是更好

这里不建议使用单例,给的用例有点问题,你这么想这个问题,假设如果我一个协程内同时存在两个 Saga 是否就出问题了 eg:

 public function complexOperation(Saga $saga1, Saga $saga2)
{

}

public function complexOperation()
{
$saga1 = new Saga();
$saga2 = new Saga();
}

这样也有问题, Saga对象里面 add方法 底层也是 协程上下文保存的;

@PandaLIU-1111 PandaLIU-1111 merged commit 66a2998 into dtm-php:master Oct 28, 2025
0 of 6 checks passed
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

Successfully merging this pull request may close these issues.

2 participants