TransportImpl::sendNextFrameToMediaTxSocket schedules callback without checking for work

[thirtytwobits]

            auto* const send_failure = cetl::get_if<ITxSocket::SendResult::Failure>(&send_result);
            if (nullptr == send_failure)
            {

here we've successfully sent a frame. The next clause handles what to do next...

                const auto sent = cetl::get<ITxSocket::SendResult::Success>(send_result);
                if (sent.is_accepted)
                {
                    popAndFreeUdpardTxItem(&media.udpard_tx(), tx_item, false /* single frame */);

In this condition the transfer was accepted and so removed from the tx queue.

                }

                // If needed schedule (recursively!) next frame for sending.
                // Already existing callback will be called by executor when TX socket is ready to send more.
                //
                if (!media.txSocketState().callback)
                {

If we get here we are supposing there's more to transmit but we haven't checked. We could be scheduling more work without any work to do.

If we haven't gotten here (if the callback was still set) what does this mean? We haven't rescheduled anything so is this callback going to fester in the schedule?

                    media.txSocketState().callback =
                        tx_socket.registerCallback([this, &media, &tx_socket](const auto&) {
                            //
                            sendNextFrameToMediaTxSocket(media, tx_socket);
                        });
                }
                return;
            }

[thirtytwobits]

The relationship between scheduling work and registering callbacks is opaque to users. It's still a mystery to me how the two things are related.

[thirtytwobits]

Also note that this appears to cause the udp implementation to come to a halt if there is not any additional work to be done. Specifically we are now stuck when entering TransportImpl::sendAnyTransfer(const AnyUdpardTxMetadata::Variant&, const PayloadFragments) because line 370 always evaluates to true:

                    // No need to try to send next frame when previous one hasn't finished yet.
                    if (!media.txSocketState().callback) // < this remains true because we set a callback
                                                         //   expecting more tx work but there was none 
                                                         //   so we never removed it.
                    {
                        sendNextFrameToMediaTxSocket(media, tx_socket);
                    }

[thirtytwobits]

I'm also wondering if we need to actually schedule callbacks when we add them. It looks like we're gunking up the schedule because we always insert things to happen at TimePointNever. So...

media.txSocketState().callback =
    tx_socket.registerCallback([this, &media, &tx_socket](const auto&) {
    sendNextFrameToMediaTxSocket(media, tx_socket);
});
++ > media.txSocketState().callback.schedule(IExecutor::Callback::Schedule::Once{executor_.now()});

which begs the question, why "register callback"? Why not just:

media.txSocketState().callback =
    tx_socket.scheduleCallback([this, &media, &tx_socket](const auto&) {
    sendNextFrameToMediaTxSocket(media, tx_socket);
}, IExecutor::Callback::Schedule::Once{executor_.now()});

?