Add a test to check issue #1366 and use Debug mode

ibc commented 5 months ago

Temporal PR just to test https://github.com/versatica/mediasoup/issues/1366 in different machines.

CC @jmillan @shaymagsumov

Results using `absl::btree_set`

SECTION("absl::btree_set")
{
    absl::btree_set<uint16_t, RTC::SeqManager<uint16_t>::SeqLowerThan> recoveredList;

    recoveredList.insert(10000);
    recoveredList.insert(40000);
    recoveredList.insert(60000);

    REQUIRE(recoveredList.size() == 3);
}

Here we can see that, when compiled in debug mode, mediasoup-worker CI fail in all OS and archs and compilers (in all but Windows) due to the transitivity violation, i.e. comp(a,b) && comp(b,c) -> comp(a,c).

All CI fail: https://github.com/versatica/mediasoup/actions/runs/8612477995/

Results using `std::set`

SECTION("std::set")
{
    std::set<uint16_t, RTC::SeqManager<uint16_t>::SeqLowerThan> recoveredList;

    recoveredList.insert(10000);
    recoveredList.insert(40000);
    recoveredList.insert(60000);

    REQUIRE(recoveredList.size() == 3);
}

Despite C++ containers are supposed to also require transitivity (https://en.cppreference.com/w/cpp/container/set, https://en.cppreference.com/w/cpp/named_req/Compare), the same transitivity violation doesn't affect standard C++ STD containers when compiled in debug mode. Why? No idea, but it doesn't fail in any OS/arch/compiler.

All CI passes: https://github.com/versatica/mediasoup/actions/runs/8612301409/

Conclusion

Ok, we use abseil containers to gain more performance.
But they are hell.
We SHOULD be able to run mediasoup-worker in debug mode without crashes. Otherwise, what do we want the debug mode for? To detect problems? How are we gonna detect problems if we are literally introducing problems when we run mediasoup-worker in debug mode due to the required transitivity in abseil containers? It doesn't make any sense.
So IMHO we MUST NOT use abseil conatiners with compare functions that do not honor the transitivity requirement, and this is the task that must be done.

jmillan commented 5 months ago

I agree, let's not use abseil containers when the comparison function is needed.

ibc commented 5 months ago

So I'm closing this temp PR and will make one with the agreed changes.

ibc commented 5 months ago

PR done: https://github.com/versatica/mediasoup/pull/1369

versatica / mediasoup