How to handle re-entrant functions?

[DeveloperPaul123]

Following up from the discussion in #103, I'm experiencing an interesting problem while using a "heartbeat" with a QSerialPort. I'm note 100% sure if I'm using this term correctly, but it seems the sendData() function below is re-entrant. Based on the discussions in #103, we have the heartbeat coroutine polling the device but periodically we also need to send auxiliary commands to our device. However, when we do this, we interrupt the heartbeat before it finishes reading the response or we get the default command response instead of the true response when sending data directly using sendData().

Previously it was stated

Regarding sending commands, it's safe to do so (as long as you are on the same thread as the other coroutines), since either the command() coroutine is running, or the heartbeat coroutines is running (or the event loop is just looping), there's no chance of a race. With serial port, I think it's also safe to assume that even if you send "Command A" immediatelly followed by "Command B" you will first receive "Response A" and then "Response B".

But I'm not sure this is actually the case unless I'm doing something wrong. Could how we're reading from the SerialPort be causing this issue?

Our basic setup is the following:

QCoro::Task<> CommunicationDevice::heartbeat()
{
    QTimer timer;
    timer.start(static_cast<int>(std::round(1000.0 / update_frequency_.get())));
    while(device_->isOpen())
    {
        co_await timer;
        if(!device_->isOpen()) break;

        if(auto result = co_await sendData(default_command_))
        {
            default_command_processor_(result.value());
        }
        else
        {
            qCritical() << "Error occurred: " << result.error().reason;
        }
    }
}


QCoro::Task<Expected<QString, CommunicationError>> CommunicationDevice::sendData(const QString& data)
{

    long long bytes_written = -1;
    const auto send_buffer = data.toUtf8();
    device()->write(send_buffer);
    const auto& device_mode = device()->openMode();
    const auto& is_unbuffered = device_mode.testFlag(QIODevice::Unbuffered);
    if(const auto& bytes_written_opt = co_await qCoro(device()).waitForBytesWritten(-1))
        bytes_written = bytes_written_opt.value();

    // detect write error
    if(bytes_written != send_buffer.size() && !is_unbuffered)
    {
        qWarning() << "Bytes written mismatch! Wrote " << bytes_written << ", expected " << send_buffer.size();
        co_return tl::make_unexpected(CommunicationError{device()->errorString()});
    }

    // device is unbuffered so we expected the bytesWritten to be 0, check if we can read anything, if we can't then
    // that's actually an error
    if(is_unbuffered && device()->bytesAvailable() == 0)
    {
        qWarning() << "Unbuffered device write/read failed.";
        co_return tl::make_unexpected(CommunicationError{device()->errorString()});
    }

    // read from the device
    const auto reply = co_await read_strategy_(device());
    co_return reply;
}

We want to ensure that when we send the default command via the heartbeat() that we always get the default response and when we send auxiliary commands directly via sendData() we get the reponse for that command only. Any help is much appreciated.

[danvratil]

Hi Paul,

just to make sure I understand the problem correctly, what you are seeing is

1. heartbeat calls sendData()
2. sendData() writes data to device and suspends while awaiting read_strategy_(), returning to event loop
3. sendData() is invoked from elsewhere to send an auxiliary command
4. the second invocation of sendData() writes data to device before reply to the first data has arrived

This is a problem inherent to any asynchronous code, even if you were not using coroutines, then you would most likely be connected()ed to the device's readyRead() signal, so after the first write, before the signal would be emitted, it would be possible for another write command to be issued.

In those scenarios I usually end up using a command queue - instead of directly invoking sendData(), I would enqueue the command (and receive a future or awaitable in return) into the command queue. If there's no ongoing write/read operation, the queue would invoke sendData() immediately, otherwise it would just append the command to a list of pending commands. When the currently ongoing operation receives response, it would complete the associated awaitable (so that the code that enqueued the particular command would receive the relevant response), and then check if there is another command in the queue to send. This way you can have multiple coroutines that are not aware of each other to request data to be sent, but you are guaranteed the data will be sent out in the right order and response is always received before next command is sent out.

If you are on Qt6 (sorry, I can't remember if you said you were using Qt5 or Qt6), you could just use QPromise inside the command queue and return an associated QFuture to the caller (QCoro has support for awaiting QFuture). If you are on Qt5, you can write a custom simple promise-awaitable pair.

[danvratil]

Ah, I have not realized the screenshot in the previous comment from from QCoro code, sorry for the confusion :-)

I'll try to reduce it to some small reproducer case within QCoro and get a fix.

[danvratil]

Do you have any more details regarding the crash? What exception is thrown, possibly backtrace?

[DeveloperPaul123]

I'm not able to generate the crash at the moment as I have moved on to some other work, but there was an access violation being thrown in the qcoro signal helper here.

[danvratil]

So far I was unable to reproduce the crash. I'll need more info to go on - at least a backtrace and if you are able to build and run your codebase under ASAN, then getting an ASAN report after the crash would help a lot.

[DeveloperPaul123]

I'll see what I can do. Thanks!