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

Provides GracefulShutdown functionality #1078

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

OlegDokuka
Copy link
Member

closes #988

Signed-off-by: Oleh Dokuka [email protected]
Signed-off-by: Oleh Dokuka [email protected]

@OlegDokuka OlegDokuka changed the base branch from master to 1.1.x November 11, 2022 13:34
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

Great changes!

There are 5 sinks passed around for coordination, in addition to 2 more Monos for when both sides are completed or closed. It takes some effort to trace their usages and I'm thinking we could encapsulate this behavior a bit to make the graceful shutdown code a bit more readable and intuitive.

As far as I can see this is primarily requester side driven, but the responder does need to be told when graceful shutdown begins, and it also needs to be able to communicate when it is done on its own side.

Let's say we have a package private contract like this:

interface GracefulShutdownTracker {

  Mono<Void> onGracefulShutdownStarted();

  Sinks.Empty<Void> responderCompletedSink();

  Sinks.Empty<Void> responderClosedSink();

}

Then inside RSocketRequester:

class RSocketRequester extends RequesterResponderSupport implements RSocket {

  private final DefaultGracefulShutdownTracker shutdownTracker = new DefaultGracefulShutdownTracker();

  // ...

  public GracefulShutdownTracker getGracefulShutdownTracker() {
    return gracefulShutdownTracker;
  }


  private final static class DefaultGracefulShutdownTracker implements GracefulShutdownTracker {

    private final Sinks.Empty<Void> startedSink = Sinks.unsafe().empty();

    private final Sinks.Empty<Void> completedSink = Sinks.unsafe().empty();
    private final Sinks.Empty<Void> closedSink = Sinks.unsafe().empty();

    private final Sinks.Empty<Void> responderCompletedSink = Sinks.unsafe().empty();
    private final Sinks.Empty<Void> responderClosedSink = Sinks.unsafe().empty();

    public Mono<Void> onGracefulShutdownStarted() {
      return startedSink.asMono();
    }

    @Override
    public Sinks.Empty<Void> responderCompletedSink() {
      return responderCompletedSink;
    }

    @Override
    public Sinks.Empty<Void> responderClosedSink() {
      return responderClosedSink;
    }

    Mono<Void> onBothCompleted() {
      return Mono.whenDelayError(
              responderCompletedSink.asMono(),
              completedSink.asMono());
    }

    Mono<Void> onBothClosed() {
      return Mono.whenDelayError(
              responderClosedSink.asMono(),
              closedSink.asMono());
    }
  }

}

Now RSocketRequester has all the Sinks and Monos it needs access internally to do its work, and also declares a GracefulShutdownControl getter that exposes what the responder needs to do its work.

This makes it clear who has what responsibility, it removes the need to pass local variables around, replacing those with a single container, and it's an opportunity to shorten sink names, to give monos a name, etc.

Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

One more note. Currently, RequestResponderSupport has an onGracefulShutdownSink as a constructor initiailized field.

In my proposal above, this could be replaced with an abstract method, something like void GracefulShutdownCompleted() which the requester and the responder would implemented accordingly based on what's available to them.

@OlegDokuka OlegDokuka force-pushed the enhancement/gracefull-shutdown branch 2 times, most recently from 5afbd9d to 5b0f592 Compare December 1, 2022 12:15
@OlegDokuka OlegDokuka changed the base branch from 1.1.x to master April 28, 2023 10:01
Signed-off-by: Oleh Dokuka <[email protected]>
Signed-off-by: Oleh Dokuka <[email protected]>
Signed-off-by: OlegDokuka <[email protected]>
@OlegDokuka OlegDokuka force-pushed the enhancement/gracefull-shutdown branch from 5049c36 to 3bf4de8 Compare April 29, 2023 06:03
@OlegDokuka OlegDokuka added this to the 1.2.0-M1 milestone Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for graceful shutdown
2 participants