Potential issue with OnceCallback

[abtink]

I was recently looking at the Mdns::Publisher implementation and its use of OnceCallback class in callback.hpp.

I think there may be an issue with the implementation, and it may not work correctly with all STL library implementations. The implementation seems to indirectly assume that a move constructor or a move assignment will clear the rvalue reference from which it moves. However, this is not guaranteed by the C++ standard and behavior is tied to specific class and its implementaion.

ot-br-posix/src/common/callback.hpp

Lines 82 to 89 in 34c3899

	R operator()(Args... args) &&
	{
	// Move `this` to a local variable to clear internal state
	// before invoking the callback function.
	OnceCallback cb = std::move(*this);
	return cb.mFunc(std::forward<Args>(args)...);
	}

We are using std::function<>. I found this related to this type which does not guarantee to clear (null) the moved-from rvalue reference:

(3-4) Copies (3) or moves (4) the target of other to the target of *this. If other is empty, *this will be empty after the call too. For (4), other is in a valid but unspecified state after the call.

ref: https://en.cppreference.com/w/cpp/utility/functional/function/function

[abtink]

I noticed there seems to be a unit test to cover this test_once_callback.cpp.

To double-check I wrote a test-case for this:

typedef OnceCallback<void(void)> SimpleOnceCallback;

static uint16_t gCallCaounter = 0;

void TestFunc(void)
{
    gCallCaounter++;
    printf("TestFunc() is called - gCallCaounter=%u\n", gCallCaounter);
}

void InvokeCallbackTwice(SimpleOnceCallback &&aCallback)
{
    printf("In InvokeCallbackTwice()\n");
    printf("  aCallback.IsNull() = %d\n", aCallback.IsNull());
    std::move(aCallback)();
    printf("  aCallback.IsNull() = %d\n", aCallback.IsNull());
    std::move(aCallback)();
}


int main(void)
{
    SimpleOnceCallback callback(TestFunc);

    printf("callback.IsNull() = %d\n", callback.IsNull());
    InvokeCallbackTwice(std::move(callback));
    printf("callback.IsNull() = %d - After InvokeCallbackTwice() \n", callback.IsNull());
    InvokeCallbackTwice(std::move(callback));

    VerifyOrQuit(gCallCaounter == 1);
}

Running this on Mac-book (with xcode command lines compilier), I get this (which show that OnceCallback can invoke multiple timesl):

callback.IsNull() = 0
In InvokeCallbackTwice()
  aCallback.IsNull() = 0
TestFunc() is called - gCallCaounter=1
  aCallback.IsNull() = 0
TestFunc() is called - gCallCaounter=2
callback.IsNull() = 0 - After InvokeCallbackTwice() 
In InvokeCallbackTwice()
  aCallback.IsNull() = 0
TestFunc() is called - gCallCaounter=3
  aCallback.IsNull() = 0
TestFunc() is called - gCallCaounter=4

FAILED main:286 - VerifyOrQuit(gCallCaounter == 1) 

On a linxu (unbunu) machine, running the same test gives the following (here the OnceCallback seem to work correctly):

callback.IsNull() = 0
In InvokeCallbackTwice()
  aCallback.IsNull() = 0
TestFunc() is called - gCallCaounter=1
  aCallback.IsNull() = 1
terminate called after throwing an instance of 'std::bad_function_call'
  what():  bad_function_call
Aborted

[abtink]

Should be resolved by #1913.