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

throttle: Minimal destructor fix for Luminous #16661

Merged
merged 1 commit into from Jul 31, 2017

Conversation

Projects
None yet
3 participants
@adamemerson
Copy link
Contributor

adamemerson commented Jul 28, 2017

Get rid of the undefined behavior of destroying condition variables while they're being waited on

@gregsfortytwo
Copy link
Member

gregsfortytwo left a comment

Is SimpleThrottle okay? There's nothing here that touches it and I didn't see anything apparent about why its signal waiter won't break in the same way.

I'm a little concerned about the tests since they don't seem to have caught the mis-matched ++waiters in OrderedThrottle.

And there's the exceptions thing, but the rest of the changes look good. I didn't know about make_scope_guard(); that's pretty nice to have in the toolkit instead of custom-building an RAII struct every time!

m_cond.Wait(m_lock);
++waiters;

This comment has been minimized.

Copy link
@gregsfortytwo

This comment has been minimized.

Copy link
@adamemerson

adamemerson Jul 28, 2017

Author Contributor

WELL, that's what I get for copying my diff without paying close enough attention.

} while ((_should_wait(c) || cv != cond.begin()) && !shutting_down);
if (shutting_down) {
shutdown_clear.SignalOne();
throw waiting_destructed("Throttle destructed while we waited.");

This comment has been minimized.

Copy link
@gregsfortytwo

gregsfortytwo Jul 28, 2017

Member

Uh, we don't use exceptions.[1] Is there some reason not to just make this an assert?

[1]: (Except that we do in bufferlist decode, but that's the only place unless something's changed, and they're outlawed by https://google.github.io/styleguide/cppguide.html#Exceptions)

This comment has been minimized.

Copy link
@adamemerson

adamemerson Jul 28, 2017

Author Contributor

We use them in RGW, too.

The reason for not making it an assert was so we could catch it in the Destructor test and make sure it was signaled properly. We could use a death test instead or just get rid of the Destructor test entirely. (My inspiration for this patch was that the Destructor test was hanging up when I ran make check.)

What's the rationale for outlawing exceptions in our project? (Google has Hysterical Reasons, but we don't necessarily have the same concerns.)

This comment has been minimized.

Copy link
@gregsfortytwo

gregsfortytwo Jul 28, 2017

Member

Mostly we don't like the way it messes around with control flow. We also use the google style guide as our baseline and nobody has ever made a persuasive argument for breaking with it in this case.

(Also, it's polite to users of our libraries not to make them play with exceptions.)

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 28, 2017

I just missed SimpleThrottle when I was moving my patch over, I'll catch that in a moment.

I think Throttle is the only one that has a test for this case. I can write ones for the others.

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 28, 2017

(Death Tests are across threads are pretty hairy, though.)

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 28, 2017

Ah, when I was doing this last night I thought the m_current assertion in SimpleThrottle checked for waiters, but I was mistaken about that. One moment.

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 28, 2017

I didn't know about scope_guard either until Kefu pointed out that the 'ward' function I wrote duplicated its functionality :)

@adamemerson adamemerson force-pushed the adamemerson:wip-throttle-minimal branch from a457aae to 4982f47 Jul 28, 2017

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 28, 2017

There, I think I've addressed everything.

@gregsfortytwo gregsfortytwo added this to the luminous milestone Jul 28, 2017

@gregsfortytwo

This comment has been minimized.

Copy link
Member

gregsfortytwo commented Jul 28, 2017

These make check errors are getting tedious, but they look like our current noise. :(

Reviewed-by: Greg Farnum gfarnum@redhat.com

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 29, 2017

What the heck, Jenkins? Retest this, please.

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 29, 2017

Why are you doing this to me, Jenkins? Retest this, if you please.

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 29, 2017

Jenkins, retest this please.

@@ -58,6 +59,8 @@ class ThrottleTest : public ::testing::Test {
}
};

static void destruct_throttle() {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 30, 2017

Contributor

@adamemerson what is this method for? did you plan to extract the common piece into it and reuse it?

@tchaikov
Copy link
Contributor

tchaikov left a comment

lgtm modulo the mysterious empty static method.

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 31, 2017

Does this mean I can merge? I'm never quite sure how many of the random 'dead' and 'failed' things on Pulpito are blockers and how many are 'Our QA system is flaky'.

@adamemerson

This comment has been minimized.

Copy link
Contributor Author

adamemerson commented Jul 31, 2017

@tchaikov It's a leftover. I'll get rid of it. I had originally meant to templatize the test function and reuse it, but all the throttles have different interfaces so that didn't work.

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jul 31, 2017

@adamemerson yeah, please merge it once destruct_throttle() is removed. i checked all the test failures. they are either known issues or issues being addressed by PRs not yet merged. i was just too lazy to list them all.

@adamemerson adamemerson force-pushed the adamemerson:wip-throttle-minimal branch from 4982f47 to 85da126 Jul 31, 2017

throttle: Minimal destructor fix for Luminous
Get rid of the undefined behavior of destroying condition variables
while they're being waited on.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

@adamemerson adamemerson force-pushed the adamemerson:wip-throttle-minimal branch from 85da126 to 0f52f48 Jul 31, 2017

@adamemerson adamemerson deleted the adamemerson:wip-throttle-minimal branch Jul 31, 2017

@adamemerson adamemerson merged commit 0f52f48 into ceph:master Jul 31, 2017

3 checks passed

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

adamemerson added a commit that referenced this pull request Jul 31, 2017

Merge pull request #16661 from adamemerson/wip-throttle-minimal
throttle: Minimal destructor fix for Luminous

Reviewed-by: Kefu Chai <kchai@redhat.com>
Reviewed-by: Greg Farnum <gfarnum@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.