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.

Sign up for GitHub

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

Jump to bottom

Fix memory leaks caused by throwing reader_concurrency_semaphore::consume() #12756

Merged
merged 2 commits into from
Feb 27, 2023
Merged

Fix memory leaks caused by throwing reader_concurrency_semaphore::consume() #12756

merged 2 commits into from
Feb 27, 2023

Conversation

denesb
Copy link
Contributor

@denesb denesb commented Feb 6, 2023 •
edited

Said method can now throw std::bad_alloc since aab5954. All call-sites should have been adapted in the series introducing the throw, but some managed to slip through because the oom unit test didn't run in debug mode. This series fixes the remaining unpatched call-sites and makes sure the test runs in debug mode too, so leaks like this are detected.

Fixes: #12767

@denesb denesb requested a review from tgrabiec as a code owner February 6, 2023 15:12
@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4209/

@scylladb-promoter
Copy link
Contributor

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4208/

@denesb
Copy link
Contributor Author

denesb commented Feb 7, 2023

Don't merge, v2 will be coming.

@denesb
Copy link
Contributor Author

denesb commented Feb 7, 2023

v2:

  • Don't swallow the exception in reset_memory().
  • Rebased.

avikivity
avikivity reviewed Feb 7, 2023
View reviewed changes
mutation_fragment.cc Outdated Show resolved Hide resolved
@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4221/

@denesb
Copy link
Contributor Author

denesb commented Feb 7, 2023

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4221/

Sanity Tests / cdc_test.TestCdc.test_cluster_reduction_with_cdc[Single_cluster]

@kbr-scylla is this a known flaky test? Seems completely unrelated to this PR.

@kbr-scylla
Copy link
Contributor

@kbr-scylla is this a known flaky test? Seems completely unrelated to this PR.

I personally haven't seen this test fail before.

@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4239/

@denesb
Copy link
Contributor Author

denesb commented Feb 8, 2023

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4239/

The test failed with:

E           ccmlib.node.NodeError: Error starting node node8: unable to connect to scylla-jmx port 127.0.20.8:7199

@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4254/

@denesb denesb mentioned this pull request Feb 8, 2023
Merged
@denesb
Copy link
Contributor Author

denesb commented Feb 8, 2023

v3:

  • mutation_fragment[_v2]::apply(): call reset_memory() after moved-from mf is cleaned-up

@fruch
Copy link
Contributor

fruch commented Feb 8, 2023

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4221/

Sanity Tests / cdc_test.TestCdc.test_cluster_reduction_with_cdc[Single_cluster]

@kbr-scylla is this a known flaky test? Seems completely unrelated to this PR.

I just happen on next a run

@fruch
Copy link
Contributor

fruch commented Feb 8, 2023

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4239/

The test failed with:

E           ccmlib.node.NodeError: Error starting node node8: unable to connect to scylla-jmx port 127.0.20.8:7199

this seems like a coredump in scylla-jmx
I've start noticing it a few days ago: https://github.com/scylladb/scylla-dtest/issues/3007

@scylladb-promoter
Copy link
Contributor

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4265/

@denesb
treewide: adapt to throwing reader_concurrency_semaphore::consume()
1a9fdeb 
Said method can now throw `std::bad_alloc` since aab5954. All call-sites
should have been adapted in the series introducing the throw, but some
managed to slip through because the oom unit test didn't run in debug
mode. In this commit the remaining unpatched call-sites are fixed.
@denesb
test/boost/reader_concurreny_semaphore_test: run oom protection tests…
0961a3f 
… in debug mode

Said tests require on being run with a limited amount of memory to be
really useful. When the memory amount is unexpected, they silently exit.
Which is exactly what they did in debug mode too, where the amount of
memory available cannot be controlled.
Disable the check in debug mode.
@denesb
Copy link
Contributor Author

denesb commented Feb 17, 2023

Rebased.

@scylladb-promoter
Copy link
Contributor

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4442/

@denesb
Copy link
Contributor Author

denesb commented Feb 20, 2023

@scylladb/scylla-maint ping.

1 similar comment
@denesb
Copy link
Contributor Author

denesb commented Feb 27, 2023

@scylladb/scylla-maint ping.

@scylladb-promoter scylladb-promoter merged commit 6f88dc8 into scylladb:master Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avikivity avikivity avikivity left review comments

@tgrabiec tgrabiec Awaiting requested review from tgrabiec tgrabiec is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

throwing reader_concurrency_semaphore::consume() causes memory leaks
5 participants
@denesb @scylladb-promoter @kbr-scylla @fruch @avikivity