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

common/Timer: do not add event if already shutdown #16201

Merged
merged 1 commit into from Jul 7, 2017

Conversation

Projects
None yet
3 participants
@tchaikov
Contributor

tchaikov commented Jul 7, 2017

otherwise the callback is leaked.

Fixes: http://tracker.ceph.com/issues/20432
Signed-off-by: Kefu Chai kchai@redhat.com

@tchaikov tchaikov requested review from liewegas and jdurgin Jul 7, 2017

@tchaikov tchaikov added this to the luminous milestone Jul 7, 2017

ldout(cct,10) << "add_event_at " << when << " -> " << callback << dendl;
ldout(cct,10) << __func__ << " " << when << " -> " << callback << dendl;
if (!thread) {

This comment has been minimized.

@jdurgin

jdurgin Jul 7, 2017

Member

there's a window when the lock isn't held during shutdown but the thread is joined, and not set to NULL yet - checking for stopping here instead should avoid that

common/Timer: do not add event if already shutdown
otherwise the callback is leaked.

Fixes: http://tracker.ceph.com/issues/20432
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 7, 2017

@jdurgin fixed and repushed.

@jdurgin

jdurgin approved these changes Jul 7, 2017

@tchaikov tchaikov added the needs-qa label Jul 7, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 7, 2017

@liewegas

This looks fine, although I worry about the callers that are submitting timer events after the timer is shut down..

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 7, 2017

The trace show a return 1 which is close to the failure : /home/jenkins-build/build/workspace/ceph-pull-requests/src/test/osd/osd-dup.sh:45: TEST_filestore_to_bluestore: returned 1

and all the jobs in the test are green for the first time!

@tchaikov tchaikov merged commit 9099d56 into ceph:master Jul 7, 2017

3 of 4 checks passed

make check make check failed
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check (arm64) make check succeeded
Details

@tchaikov tchaikov deleted the tchaikov:wip-20432 branch Jul 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment