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

Add paused method to PauseFailControl #6724

Merged
merged 1 commit into from
Jun 1, 2016

Conversation

nathanielmanistaatgoogle
Copy link
Member

This will be used in tests that want to ensure before proceeding that
the system under test has progressed to the control point.

@nathanielmanistaatgoogle
Copy link
Member Author

(Python is green.)

self._condition.notify_all()

def paused(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can this be named wait_for_pause?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about block_until_paused?

@kpayson64
Copy link
Contributor

Maybe just add a small comment that Control's can't be shared because it seems like that is a change in behavior.


@contextlib.contextmanager
def pause(self):
"""Pauses code under control while controlling code is in context."""
with self._condition:
self._paused = True
self._pause = True
yield
with self._condition:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make paused be private, and call it after the yield? Or are there possible cases where the control never gets paused but we want to unpause it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're asking here - read up on what @contextlib.contextmanager does to this function?

(One of the reasons I don't like decorators in Python is that they often change code in radical ways.)

I think this object makes sense with callable public methods and non-callable private fields and I'm not at all seeing what a private method would do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading @contextlib.contextmanager, what I said doesn't make sense...

What I intended is something along these lines:

@contextlib.contextmanager
with self._condition:
  self._pause = True
self._block_until_paused()
yield
self._pause = False
self._condition.notify_all()

Then anytime we have

with control.pause()
  #Do some stuff

We know that control is actually paused without needing to call block_until_paused

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be really bad because the way this object is used is

with control.pause():
  <set in motion the behavior under test that would lead to the system under test being paused>
  <either call control.block_until_paused or don't>
  <maybe do some other stuff>

. With the semantics you suggest, that code would have to be changed to

<set in motion the behavior under test that would lead to the system under test being paused>
with control.pause():
  <either call control.block_until_paused or don't>
  <maybe do some other stuff>

and that would create a race condition in which the system under test could pass the control point (where it calls control.control()) before the test system enters the control.pause() context.

Concretely, look at the way this is used in expiration tests: we say that the service-side application should pause, then we invoke the RPC, then we chill out until we see the RPC time out, and then we leave the pause context and allow the service-side application to return from its RPC method handler. With the behavior you propose, the test would have to invoke the RPC before entering the pause context and the service-side application could finish servicing the RPC method before the test enters the pause context.

Right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks for the context.

This method will be used in tests that want to ensure before
proceeding that the system under test has progressed to the control
point.
@nathanielmanistaatgoogle
Copy link
Member Author

Comment addressing what I think you meant about sharing added.

Back over to you for more review.

@kpayson64
Copy link
Contributor

LGTM

@nathanielmanistaatgoogle
Copy link
Member Author

(Python is green.)

@jtattermusch jtattermusch merged commit f7d95c3 into grpc:master Jun 1, 2016
@nathanielmanistaatgoogle nathanielmanistaatgoogle deleted the paused branch June 15, 2016 02:41
@lock lock bot locked as resolved and limited conversation to collaborators Jan 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants