Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement LWG-2309 mutex::lock() should not throw device_or_resource_busy #3469

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Feb 15, 2023

Fixes #75. Fixes #3407.


This PR is currently subject to further changes, but decision is probably needed.

  1. In the description of LWG-2309, SG1 suggested that resource_deadlock_would_occur should be thrown in the UB case; POSIX specifies EAGAIN (corresponding to resource_unavailable_try_again) on ownership level overflowing. While @BillyONeal preferred abort() for UB, and mentioned that SG1 suggested terminate() for overflowing (which, however, needs another LWG issue).
  2. When implementing this, I strongly felt that it'd be better to divorce mutex::lock and recursive_mutex::lock because they have entirely different failing cases (same for try_lock). I guess they should be different functions.
  3. The current changes are header-only. But I suspect that it'd be better to change mutex.cpp, although this will affect redist.
  4. Test coverage is missing, because I wrote a test program like the following which succeeded but spent over 10 minutes running on my machine (in debug build). Most of the time were spent by operations of recursive_timed_mutex. While operations of recursive_mutex were far faster, they still took over 1 minute to run.

The extremely time-consuming test program:

#include <cassert>
#include <cstdint>
#include <mutex>
#include <system_error>

using namespace std;

template <class Mtx>
void test_overflowing_lock_levels() {
    constexpr uint64_t large_count = 0x20'0000'0000ULL;

    Mtx m{};
    bool thrown  = false;
    uint64_t cnt = 0ULL;

    try {
        for (; cnt != large_count; ++cnt) {
            m.lock();
        }
    } catch (const std::system_error& e) {
        thrown = true;
        assert(e.code().value() == static_cast<int>(std::errc::resource_unavailable_try_again));
    } catch (...) {
        assert(false);
    }

    assert(thrown);

    while (cnt != 0) {
        --cnt;
        m.unlock();
    }
}

int main() {
    test_overflowing_lock_levels<recursive_mutex>();
    test_overflowing_lock_levels<recursive_timed_mutex>();
}

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner February 15, 2023 03:54
@github-actions github-actions bot added this to Initial Review in Code Reviews Feb 15, 2023
@StephanTLavavej StephanTLavavej added bug Something isn't working LWG Library Working Group issue labels Feb 15, 2023
stl/inc/mutex Outdated Show resolved Hide resolved
stl/inc/mutex Outdated Show resolved Hide resolved
stl/inc/mutex Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Feb 15, 2023
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Final Review in Code Reviews Feb 15, 2023
@barcharcraz barcharcraz self-assigned this Feb 16, 2023
stl/inc/mutex Outdated Show resolved Hide resolved
@frederick-vs-ja frederick-vs-ja mentioned this pull request Feb 16, 2023
@StephanTLavavej StephanTLavavej moved this from Final Review to Initial Review in Code Reviews Feb 16, 2023
@StephanTLavavej StephanTLavavej self-assigned this Feb 16, 2023
@barcharcraz barcharcraz moved this from Initial Review to Final Review in Code Reviews Feb 16, 2023
@barcharcraz barcharcraz removed their assignment Feb 16, 2023
stl/inc/mutex Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Feb 16, 2023
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Feb 16, 2023
@StephanTLavavej StephanTLavavej self-assigned this Feb 22, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit a5606c3 into microsoft:main Feb 23, 2023
Code Reviews automation moved this from Ready To Merge to Done Feb 23, 2023
@StephanTLavavej
Copy link
Member

Thanks for implementing one of the oldest outstanding LWG issues! 🎉 🚀 🔒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working LWG Library Working Group issue
Projects
No open projects
4 participants