Fixed conditon on remove token reference.

[kalman5]

Previous version was incorrectly not accounting for source reference counter but twice the token reference counter.

[josuttis]

looks good. Thanks for catching this.
Nico

[Grumpfy]

Hi Gaetano,

Do you have any repro steps for the issue you tried to solve here? Because I think this introduced the issue #42

[kalman5]

It was just pure satic code analysis, if the new version has an issue then the same issue is in __remove_source_reference ( same condition )

…

On Wed, Jun 16, 2021 at 10:39 AM Stephane Guy ***@***.***> wrote: Hi Gaetano, Do you have repro steps for the issue you tried to solve here? Because I think this introduced the issue #42 <#42> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#39 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPIUZMFAGYUEJZF7I7R5BTTTBPM3ANCNFSM4ZSC3BGQ> .

-- cpp-today.blogspot.com

[Grumpfy]

Ok, the two methods need to have different conditions, this is not a bug.

Here are the 3 methods that can delete the internal state:
the currently incorrect one

jthread/source/stop_token.hpp

Lines 53 to 59 in 0fa8d39

	void __remove_token_reference() noexcept {
	auto __oldState =
	__state_.fetch_sub(__token_ref_increment, std::memory_order_acq_rel);
	if (__oldState < (__token_ref_increment + __source_ref_increment)) {
	delete this;
	}
	}

and the ones that are correct

jthread/source/stop_token.hpp

Lines 65 to 71 in 0fa8d39

	void __remove_source_reference() noexcept {
	auto __oldState =
	__state_.fetch_sub(__source_ref_increment, std::memory_order_acq_rel);
	if (__oldState < (__token_ref_increment + __source_ref_increment)) {
	delete this;
	}
	}

jthread/source/stop_token.hpp

Lines 280 to 289 in 0fa8d39

	void __unlock_and_decrement_token_ref_count() noexcept {
	auto __oldState = __state_.fetch_sub(
	__locked_flag + __token_ref_increment, std::memory_order_acq_rel);
	// Check if new state is less than __token_ref_increment which would
	// indicate that this was the last reference.
	if (__oldState <
	(__locked_flag + __token_ref_increment + __token_ref_increment)) {
	delete this;
	}
	}

And this is the values of the flags of the internal state:

jthread/source/stop_token.hpp

Lines 291 to 295 in 0fa8d39

	static constexpr std::uint64_t __stop_requested_flag = 1u;
	static constexpr std::uint64_t __locked_flag = 2u;
	static constexpr std::uint64_t __token_ref_increment = 4u;
	static constexpr std::uint64_t __source_ref_increment =
	static_cast<std::uint64_t>(1u) << 33u;

The logic is always to delete the state when the new state is less or equal to __stop_requested_flag+ __locked_flag which is equal to 3, hence it is equivalent to strictly less than __token_ref_increment which is equal to 4. In the 3 concerned methods the code uses fetch_sub that subtract a value from the atomic and return the previous value of the state.
Meaning the right check is always something of the form:

    auto __oldState =
        __state_.fetch_sub(SOME_VALUE, std::memory_order_acq_rel);
    if (__oldState < (__token_ref_increment + SOME_VALUE)) {
      delete this;
    }

[kalman5]

Actually, you are correct.

To summarize. The delete should happen when there are not more Sources or Tokens, so the reference counter reached 0. In this case 0 means that atomic contains only the locked or stopped bit on = 3.

NewCounter <= 3
NewCounter < 4
NewCounter < __token_ref_increment

NewCounter = OldCounter - X

OldCounter -x < __token_ref_increment
OldCounter < __token_ref_increment + x

tbh to avoid future confusion I would add:

static constexpr std::uint64_t __emptyState = __stop_requested_flag  + __locked_flag; 

and then convert those conditions as:

auto __oldState =
        __state_.fetch_sub(SOME_VALUE, std::memory_order_acq_rel);
    if (__oldState <= (__emptyState  + SOME_VALUE)) {
      delete this;
    }

[josuttis]

thanks a lot for your work. I am currently very busy, but I come back Nico Am 22. Juni 2021 22:25:58 MESZ schrieb Gaetano ***@***.***>:

Actually, you are correct. To summarize. The delete should happen when there are not more Sources or Tokens, so the reference counter reached 0. In this case 0 means that atomic contains only the locked or stopped bit on = 3. NewCounter <= 3 NewCounter < 4 NewCounter < __token_ref_increment NewCounter = OldCounter - X OldCounter -x < __token_ref_increment OldCounter < __token_ref_increment + x tbh to avoid future confusion I would add: ``` static constexpr std::uint64_t __emptyState = __stop_requested_flag + __locked_flag; ``` and then convert those conditions as: ``` auto __oldState = __state_.fetch_sub(SOME_VALUE, std::memory_order_acq_rel); if (__oldState <= (__emptyState + SOME_VALUE)) { delete this; } ``` -- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: #39 (comment)

-- Nico Josuttis (sent from my mobile phone)