Current thread_id changing after suspending in direct execution

[Pansysk75]

Background

@hkaiser I came across this issue while investigating the current hangs in cancelable_action test.

In that test, the current thread_id is firstly obtained using hpx::this_thread::get_id() (through a reset_id helper object).
Then, we have a workloop that occasionally suspends, waiting to be interrupted (hpx::thread::interrupt(id_)).

hpx/examples/cancelable_action/cancelable_action/server/cancelable_action.hpp

Lines 81 to 94 in bc993dd

	void do_it()
	{
	reset_id r(*this); // manage thread id
	while (true)
	{
	// do something useful ;-)
	delay(1000);
	// check whether this thread was interrupted and throw a special
	// exception if it was interrupted
	hpx::this_thread::suspend(); // interruption_point();
	}
	}

Issue

I found that calling hpx::this_thread::get_id() before and after hpx::this_thread::suspend() would often return a different id.
I believe this is due to the way yielding is handled when utilizing direct execution:

hpx/libs/core/coroutines/include/hpx/coroutines/detail/coroutine_stackful_self_direct.hpp

Lines 34 to 39 in bc993dd

	// direct execution of a thread needs to use the executing context for
	// yielding
	arg_type yield_impl(result_type arg) override
	{
	return next_self_->yield_impl(arg);
	}

coroutine_stackful_self_direct falls back to next_self_ (its calling context) for yielding, which will fail to correctly restore the state after yielding:

hpx/libs/core/coroutines/include/hpx/coroutines/detail/coroutine_stackful_self.hpp

Lines 32 to 42 in bc993dd

	arg_type yield_impl(result_type arg) override
	{
	this->pimpl_->bind_result(arg);
	{
	reset_self_on_exit on_exit(this);
	this->pimpl_->yield();
	}
	return *pimpl_->args();
	}

If I understand correctly, reset_self_on_exit will cause the static variable local_self_ to refer to the "simple" (coroutine_stackful_self) coroutine after yielding (and not to the direct coroutine where it used to before the yield, see the use of reset_self_on_exit on coroutine_impl::invoke_directly()).

The solution is probably simple, but I just barely got to understanding how these coroutines work, so I just hope I got things right fttb.

[hkaiser]

@Pansysk75 Yep, that's caused by the direct execution patch. cancelable_action should call get_outer_id instead (here:

hpx/examples/cancelable_action/cancelable_action/server/cancelable_action.hpp

Line 61 in bc993dd

outer_.id_ = hpx::this_thread::get_id();

). Excellent catch!

[hkaiser]

@Pansysk75 Yep, that's caused by the direct execution patch. cancelable_action should call get_outer_id instead (here:

hpx/examples/cancelable_action/cancelable_action/server/cancelable_action.hpp

Line 61 in bc993dd

outer_.id_ = hpx::this_thread::get_id();

). Excellent catch!

I just realized that we don't have that - what an oversight! It should be added here:

hpx/libs/core/threading/include/hpx/threading/thread.hpp

Line 284 in bc993dd

HPX_CORE_EXPORT thread::id get_id() noexcept;

and here:

hpx/libs/core/threading/src/thread.cpp

Lines 386 to 389 in bc993dd

	thread::id get_id() noexcept
	{
	return thread::id(threads::get_self_id());
	}

The new code should be:

        thread::id get_outer_id() noexcept
        {
            return thread::id(threads::get_outer_self_id());
        }

[Pansysk75]

That should work, but as a user I would really wonder what "outer" refers to and why I should use that (if I didn't know the implementation details).
I was thinking of perhaps modifying the yield operation so that the state before and after the yield would be the same, if that is at all possible. That should make 'hpx::this_thread::get_id()' return some constant id (the one of the thread being "directly executed" i suppose)
Is there a good reason why a different id should be reported right after suspension? Is it a bug or is it something that we expect?

[hkaiser]

Alternatively we could make the current implementation of get_id call the get_outer_self_id instead. Not sure what this would break, though...