Skip to content

Stub and server lifecycle fixes#4269

Merged
soltanmm merged 2 commits intogrpc:release-0_12from
nathanielmanistaatgoogle:lifecycle
Dec 3, 2015
Merged

Stub and server lifecycle fixes#4269
soltanmm merged 2 commits intogrpc:release-0_12from
nathanielmanistaatgoogle:lifecycle

Conversation

@nathanielmanistaatgoogle
Copy link
Copy Markdown
Contributor

Fixes at least #3820, #529 and possibly a few other problems too.

For a termination action to properly mark an _End as having stopped it
must clear the _End's _cycle attribute. To be able to do that the
termination action must hold a reference to the _End instance.
Migrating the _termination_action behavior that creates termination
actions into the scope of the _End instance is the best way to afford
the needed instance reference.
Context management is implemented.

Stub deletion now cancels all RPCs immediately.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This appears to be the only place where the nullability of _stop_events comes into play for branching purposes, and appears to be replaceable with a cast to bool . Is there any other reason to have _stop_events be nullable and have the extra creation logic in _schedule_stop? (this is more of a just-in-case than a I-have-a-strong-opinion thing)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The non-None-ness of _stop_events indicates that the server is "stopping" (was asked to stop and was giving a positive grace). So when _end_link is non-None and _stop_events is None, that indicates that the server is fully up. Also the identity of _stop_events is used by passing it to the thread that waits during the grace period.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see that, I meant only that it looks like all that was said could just as well have been done without relying on None-ness. I don't recall ever querying you before for your opinion on None as a matter of style, s'all.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Except for the identity thing (not directly anyway). What's the situation being guarded against? Multiple stops and really bad thread scheduling?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

None is a very effective sentinel, and I do like using it in complex-state-objects to indicate "this field is not valid when the object is in this category of state".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exactly. Consider:
(1) Server is running.
(2) Server is told to stop with grace; thread is kicked off.
(3) Server is told to stop with no grace. Grace thread is unblocked on its event, but for some reason doesn't get scheduled on a CPU. Server stops.
(4) Server is restarted.
(5) Server is told to stop with long grace.
(6) Thread kicked off in (2) is finally scheduled and runs on a CPU.

Without the identity comparison, that thread would acquire the lock, see that the server is in a shutdown grace period, and complete the shutdown. With the identity comparison, the thread is able to determine that although the server is in a shutdown grace period, it is not in the same shutdown grace period as the one the thread was scheduled to conclude, and so it correctly does nothing.

soltanmm added a commit that referenced this pull request Dec 3, 2015
@soltanmm soltanmm merged commit 31c16e5 into grpc:release-0_12 Dec 3, 2015
@nathanielmanistaatgoogle nathanielmanistaatgoogle deleted the lifecycle branch January 20, 2016 00:33
@lock lock bot locked as resolved and limited conversation to collaborators Jan 29, 2019
@lock lock bot unassigned soltanmm Jan 29, 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.

3 participants