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

Plugin API changes #855

Merged
merged 7 commits into from Apr 21, 2022
Merged

Plugin API changes #855

merged 7 commits into from Apr 21, 2022

Conversation

BrynCooke
Copy link
Contributor

@BrynCooke BrynCooke commented Apr 14, 2022

Plugin API changes PR #855

Previously the Plugin trait has three lifecycle hooks: new, startup, and shutdown.

Startup and shutdown are problematic because:

  • Plugin construction happens in new and startup. This means creating in new and populating in startup.
  • Startup and shutdown has to be explained to the user.
  • Startup and shutdown ordering is delicate.

The lifecycle now looks like this:

  1. new
  2. activate
  3. drop

Users can migrate their plugins using the following:

  • Plugin#startup->Plugin#new
  • Plugin#shutdown->Drop#drop

In addition the activate lifecycle hook is now not marked as deprecated, and users are free to use it.

Implementation notes

  • Mostly this is just making new async and removing the startup and shutdown methods.
  • Configuration changes that fail will still not go live, and the router will continue to use the old state.
  • Plugins have been hoisted into the sate machine. This eliminates the RouterFactory state being combined with the StateMachine state. For state there can only be one

@netlify
Copy link

netlify bot commented Apr 14, 2022

Deploy Preview for apollo-router-docs canceled.

Name Link
🔨 Latest commit 15e2e64
🔍 Latest deploy log https://app.netlify.com/sites/apollo-router-docs/deploys/6261739dbfea5b00095133b5

@github-actions

This comment has been minimized.

@BrynCooke BrynCooke linked an issue Apr 14, 2022 that may be closed by this pull request
@BrynCooke BrynCooke changed the title Tests passing Plugin API changes Apr 14, 2022
BrynCooke pushed a commit that referenced this pull request Apr 14, 2022
Previously the Plugin trait has three lifecycle hooks: new, startup, and shutdown.

Startup and shutdown are problematic because:
* Plugin construction happens in new and startup. This means creating in new and populating in startup.
* Startup and shutdown has to be explained to the user.
* Startup and shutdown ordering is delicate.

The lifecycle now looks like this:
1. `new`
2. `activate`
3. `drop`

Users can migrate their plugins using the following:
* `Plugin#startup`->`Plugin#new`
* `Plugin#shutdown`->`Drop#drop`

In addition the `activate` lifecycle hook is now not marked as deprecated, and users are free to use it.
@BrynCooke BrynCooke self-assigned this Apr 14, 2022
@BrynCooke BrynCooke marked this pull request as ready for review April 14, 2022 13:49
@BrynCooke BrynCooke requested review from garypen, bnjjj, o0Ignition0o and Geal and removed request for garypen and bnjjj April 14, 2022 13:49
Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

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

I can't quite figure out from the review, but part of the motivation behind "previous_plugins" was so that we could safely recover from a failing configuration. Is that not a requirement now? Does the IndexMap insert change means that is now handled in there?

apollo-router/src/plugins/telemetry/mod.rs Outdated Show resolved Hide resolved
apollo-router/src/router_factory.rs Outdated Show resolved Hide resolved
apollo-router/src/router_factory.rs Show resolved Hide resolved
@BrynCooke
Copy link
Contributor Author

BrynCooke commented Apr 14, 2022

I can't quite figure out from the review, but part of the motivation behind "previous_plugins" was so that we could safely recover from a failing configuration. Is that not a requirement now? Does the IndexMap insert change means that is now handled in there?

Added some notes in the description, but here's the elevator pitch.

The user can use Drop to clean up resources. And we will ensure that plugins are not dropped until they are no longer in use (they end up in the state machine private state). Using Drop is much safer for everyone as it means that if we modify our code we don't have to make sure that every error case is handled to ensure that we call shutdown. There were several places in our code where we use ? and had not explicitly dealt with cleanup. Using Drop eliminates this issue.

The downside is that if any async work needs to be done in drop then users have to create a tokio runtime to do this. But in general this shouldn't be needed, and it's likely only to be Otel with it's weird hanging behaviour that requires it.

@garypen garypen self-requested a review April 14, 2022 17:31
@@ -18,6 +19,8 @@ use tower::{BoxError, ServiceBuilder, ServiceExt};
use tower_service::Service;
use typed_builder::TypedBuilder;

pub type Plugins = IndexMap<String, Box<dyn DynPlugin>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

using indexmap is a nice touch :)

Copy link
Contributor

@Geal Geal left a comment

Choose a reason for hiding this comment

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

this is much nicer than the previous workflow 👍

@BrynCooke BrynCooke force-pushed the bryn/plugin-lifecycle branch 2 times, most recently from 9fe95e2 to 9afd1b0 Compare April 21, 2022 12:06
bryn and others added 5 commits April 21, 2022 17:03
…and shutdown.

Startup and shutdown are problematic because:
* Plugin construction happens in new and startup. This means creating in new and populating in startup.
* Startup and shutdown has to be explained to the user.
* Startup and shutdown ordering is delicate.

The lifecycle now looks like this:
1. `new`
2. `activate`
3. `drop`

Users can migrate their plugins using the following:
* `Plugin#startup`->`Plugin#new`
* `Plugin#shutdown`->`Drop#drop`

In addition the `activate` lifecycle hook is now not marked as deprecated, and users are free to use it.
Co-authored-by: Gary Pennington <gary@apollographql.com>
@BrynCooke BrynCooke enabled auto-merge (squash) April 21, 2022 15:09
@BrynCooke BrynCooke merged commit 01f928c into main Apr 21, 2022
@BrynCooke BrynCooke deleted the bryn/plugin-lifecycle branch April 21, 2022 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Plugin startup and shutdown
4 participants