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

Fix wrong bounds of and concurrent access to counter on pipeline test #1072

Merged
merged 6 commits into from
Jan 11, 2024

Conversation

msimberg
Copy link
Collaborator

@msimberg msimberg commented Jan 9, 2024

In the particular test three read-only accesses to a pipeline are started concurrently using start_detached. Each access increments a counter, and the order in which they are incremented depends on when tasks are scheduled (since read-only access may happen concurrently). Thus the expected value of the counter in each of the three tasks is between 27 and 29. One of the tasks incorrectly had the bounds as 27 to 27 (i.e. exactly 27). Since this was the first task this would frequently be true, but is not guaranteed to be true.

Additionally, the modification of the counter in read-only sections was unprotected (and the counter was not atomic). This adds a lock to protect access to the counter.

@msimberg msimberg added the Type:Bug Something isn't working label Jan 9, 2024
@msimberg
Copy link
Collaborator Author

msimberg commented Jan 9, 2024

cscs-ci run

1 similar comment
@msimberg
Copy link
Collaborator Author

msimberg commented Jan 9, 2024

cscs-ci run

@rasolca
Copy link
Collaborator

rasolca commented Jan 9, 2024

Note as you are already fixing test_pipeline.cpp:

test/unit/common/test_pipeline.cpp:764:24: warning: implicit conversion loses integer precision: 'std::size_t' (aka 'unsigned long') to 'std::mersenne_twister_engine<unsigned int, 32, 624, 397, 31, 2567483615, 11, 4294967295, 7, 2636928640, 15, 4022730752, 18, 1812433253>::result_type' (aka 'unsigned int') [-Wshorten-64-to-32]
std::mt19937 gen(seed);

(with clang14)

@msimberg
Copy link
Collaborator Author

msimberg commented Jan 9, 2024

cscs-ci run

@msimberg
Copy link
Collaborator Author

msimberg commented Jan 9, 2024

Note as you are already fixing test_pipeline.cpp:

test/unit/common/test_pipeline.cpp:764:24: warning: implicit conversion loses integer precision: 'std::size_t' (aka 'unsigned long') to 'std::mersenne_twister_engine<unsigned int, 32, 624, 397, 31, 2567483615, 11, 4294967295, 7, 2636928640, 15, 4022730752, 18, 1812433253>::result_type' (aka 'unsigned int') [-Wshorten-64-to-32]
std::mt19937 gen(seed);

(with clang14)

Do you see that in CI? If yes, are we again missing warnings-as-errors?

@msimberg
Copy link
Collaborator Author

msimberg commented Jan 9, 2024

The test seems to fail a bit more reliably with the sleeps introduced in daa8680: https://gitlab.com/cscs-ci/ci-testing/webhook-ci/mirrors/4700071344751697/7514005670787789/-/pipelines/1132292952.

@rasolca I've also changed the seeding of the generator to something that I think will silence the warning.

@msimberg
Copy link
Collaborator Author

msimberg commented Jan 9, 2024

cscs-ci run

@msimberg msimberg marked this pull request as ready for review January 9, 2024 16:56
@msimberg msimberg requested a review from rasolca January 9, 2024 17:12
@msimberg
Copy link
Collaborator Author

There was an unrelated failure again waiting for a job to finish: https://gitlab.com/cscs-ci/ci-testing/webhook-ci/mirrors/4700071344751697/7514005670787789/-/jobs/5894331404. I've restarted the job.

@msimberg
Copy link
Collaborator Author

cscs-ci run

@msimberg
Copy link
Collaborator Author

@rasolca I've changed some, but not all, thens to transforms. In some of the tests I think it's still useful to rely on the inline execution of the continuations, and like this we hopefully get a happy mix of both inline and concurrent execution with tasks.

@rasolca rasolca added this to the V0.4.0 milestone Jan 11, 2024
@rasolca
Copy link
Collaborator

rasolca commented Jan 11, 2024

Note as you are already fixing test_pipeline.cpp:

test/unit/common/test_pipeline.cpp:764:24: warning: implicit conversion loses integer precision: 'std::size_t' (aka 'unsigned long') to 'std::mersenne_twister_engine<unsigned int, 32, 624, 397, 31, 2567483615, 11, 4294967295, 7, 2636928640, 15, 4022730752, 18, 1812433253>::result_type' (aka 'unsigned int') [-Wshorten-64-to-32]
std::mt19937 gen(seed);

(with clang14)

Do you see that in CI? If yes, are we again missing warnings-as-errors?

No, I was getting it on my laptop.

@rasolca rasolca merged commit d9f0fd7 into eth-cscs:master Jan 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:Bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants