Introduce serverWillStop life-cycle hook to plugin API

[abernix]

Similar in spirit to the serverWillStart hook of the new plugin API, we need to introduce a serverWillStop hook that can be used by a plugin to clean-up after itself.

As a concrete example, the operation registry plugin needs to call .stop() on itself to clear the interval which is created within its serverWillStart phase.

Whereas the serverWillStart is invoked from various middleware, I suspect that serverWillStop will be triggered only when code explicitly calls ApolloServer.prototype.stop (e.g., if a process.on('exit', ...) hook is declared in Node.js environments, etc.).

[abernix]

It's entirely possible that — like other "start" hooks — that the definition of such a hook should take place within an object or callback that is returned from serverWillStart.

It's plausible that the nested approach would be preferred since it would provide a natural scope to a theoretical handle that needs cleaning up. For example, with const timer = setInterval(...) within serverWillStart, having access to timer in serverWillStop is important!

I've added a help-wanted label to this, but prior to any PR, this is an example of the design considerations which needs to be had about the best pattern here.

Input appreciated!

[andrewmcgivery]

Having it as a nested function from serverWillStart I think makes sense, matches up with other Apollo patterns, and matches up with other patterns in the industry (E.g. React useEffect returning method for disposing).

I think the most important piece would be to document the "gotcha" that this would have to be manually triggered by ApolloServer.prototype.stop as I don't think that will be intuitive otherwise and there will be a lot of questions around "what triggers this event?"

[abernix]

Thanks for your perspective on this, @andrewmcgivery!

I agree with your point about the gotcha and I do think the most urgent need for this hook could first be met by documenting how to wire it up in Node.js!

[abernix]

As a nexts step / natural extension of the feature: I think it may be reasonable to, when we have process Events and we're certain we understand the dynamics of the runtime (generally speaking, Node.js), pre-wire it to invoke ApolloServer.prototype.stop automatically.

I don't have firm feelings on the pairing between Node.js process events and this serverWillStop, but beforeExit seems somewhat relevant and it's possible that it could be an async hook to allow — in the same way beforeExit allows it — to keep work on the event loop prior to actual shutdown.

That prompts the question of whether the next hook to introduce might be a synchronous serverDidStop and whether it should be wired up to exit + SIGTERM + SIGINT.

[abernix]

For me, this thinking about the nested nature begs another question of whether it makes sense to have requestDidStart (somewhere) within serverWillStart. This would be a breaking change (not too hard to re-structure for users, however!) to the current API, but here's my thinking...

Specifically, consider a case where an instance of something needs to be created by a plugin at server startup, made available to requests, and cleaned up on shutdown. Whether or not it needs to be available to the context that is received by resolvers is not specified. Some examples:

• A pool of database connections.
• A persistent connection / backhaul.
• An audit log.

I demonstrated the intention of this nested approach in a comment on #3988, but I'll quote it here:

A note about the pattern: The addition of the willResolveField hook in this PR continues with the nested approach (e.g. willResolveField is within executionDidStart is within requestDidStart) that the plugin API previous introduced and is one of its strengths. Seen in the example below, this provides a natural way to scope variables within related spans of the request life-cycle and provides narrowed TypeScript typing via the requestContext which is received along the way.

The TypeScript definitions demonstrate the precision of that narrowing and can be best seen within the apollo-server-plugin-base module:

apollo-server/packages/apollo-server-plugin-base/src/index.ts

Lines 56 to 116 in 52418a4

	export interface ApolloServerPlugin<
	TContext extends BaseContext = BaseContext
	> {
	serverWillStart?(service: GraphQLServiceContext): ValueOrPromise<void>;
	requestDidStart?(
	requestContext: GraphQLRequestContext<TContext>,
	): GraphQLRequestListener<TContext> | void;
	}
	export type GraphQLRequestListenerParsingDidEnd = (err?: Error) => void;
	export type GraphQLRequestListenerValidationDidEnd =
	((err?: ReadonlyArray<Error>) => void);
	export type GraphQLRequestListenerExecutionDidEnd = ((err?: Error) => void);
	export type GraphQLRequestListenerDidResolveField =
	((error: Error | null, result?: any) => void);
	export interface GraphQLRequestListener<
	TContext extends BaseContext = BaseContext
	> extends AnyFunctionMap {
	didResolveSource?(
	requestContext: GraphQLRequestContextDidResolveSource<TContext>,
	): ValueOrPromise<void>;
	parsingDidStart?(
	requestContext: GraphQLRequestContextParsingDidStart<TContext>,
	): GraphQLRequestListenerParsingDidEnd | void;
	validationDidStart?(
	requestContext: GraphQLRequestContextValidationDidStart<TContext>,
	): GraphQLRequestListenerValidationDidEnd | void;
	didResolveOperation?(
	requestContext: GraphQLRequestContextDidResolveOperation<TContext>,
	): ValueOrPromise<void>;
	didEncounterErrors?(
	requestContext: GraphQLRequestContextDidEncounterErrors<TContext>,
	): ValueOrPromise<void>;
	// If this hook is defined, it is invoked immediately before GraphQL execution
	// would take place. If its return value resolves to a non-null
	// GraphQLResponse, that result is used instead of executing the query.
	// Hooks from different plugins are invoked in series and the first non-null
	// response is used.
	responseForOperation?(
	requestContext: GraphQLRequestContextResponseForOperation<TContext>,
	): ValueOrPromise<GraphQLResponse | null>;
	executionDidStart?(
	requestContext: GraphQLRequestContextExecutionDidStart<TContext>,
	):
	| GraphQLRequestExecutionListener
	| GraphQLRequestListenerExecutionDidEnd
	| void;
	willSendResponse?(
	requestContext: GraphQLRequestContextWillSendResponse<TContext>,
	): ValueOrPromise<void>;
	}
	export interface GraphQLRequestExecutionListener<
	TContext extends BaseContext = BaseContext
	> extends AnyFunctionMap {
	executionDidEnd?: GraphQLRequestListenerExecutionDidEnd;
	willResolveField?(
	fieldResolverParams: GraphQLFieldResolverParams<any, TContext>
	): GraphQLRequestListenerDidResolveField | void;
	}

The block scope approach within requestDidStart seems nice to me, particularly since the variable scope of objects initialized in requestDidStart are available naturally in willResolveField and fall out of scope naturally after the request. More granularly, this seems even more powerful: a variable declared and assigned in executionDidStart only exists during the execution-phases and not at the Request level.

     requestDidStart: () => {
        const variableScopedInExecutionDidStart = ":rocket:";
        return {
          executionDidStart: () => {
            const variableScopedInExecutionDidStart = ":wave:";
            return {
              willResolveField({ source, args, context, info }) {
                return () => {
                  console.log(variableScopedInExecutionDidStart, variableScopedInExecutionDidStart)
                }
              }
            }
          }
        }
      }

Currently, requestDidStart is not nested within the serverWillStart hook. I don't think requestDidStart would go directly within serverWillStart, but within serverDidStart seems right. (serverDidStart doesn't exist yet either, and is another extension of this issue's feature ask!).

What I'm getting at is, for me, it's seeming natural (and like we've not thus embraced this the way I would expect) on the serverWillStart hook, where this pattern would seem more aligned with the nested goals:

Note: This is not supported right now!

const plugin = {
  serverWillStart: async () => {
    const pool = await getPool();
    return {
      serverDidStart: () => ({
        requestDidStart: () => ({
          /* Request-hooks here! */
        }),
        serverWillStop: async () => {
          /* This would be triggered by calling `.stop` or a TERM/INT to the process */
          await pool.flushAndClose();
          return {
            serverDidStop: () => {
              console.log(":door:")
            }
          }
        }
      })
    }
  }
};

This feels more properly tiered to me, though it does eliminate the shorter-hand approach we have now for, e.g.:

const plugin = {
  requestDidStart() {
  }
};

Thoughts?

[kubejm]

I'm still trying to familiarize myself with the internals of Apollo, but I think the approach makes sense and manages scope nicely. It seems this is effectively currying and is more inline with the functional programming paradigm.

Additionally, I'd be happy to help work this through or collaborate if my assistance could be beneficial.

[glasser]

As far as signal/shutdown handling is concerned, I'll note that the reporting agent has always defaulted to doing some cleanup on SIGINT and SIGTERM, which can be disabled by passing engine: {handleSignals: false}. I'm already working on rewriting the engine reporting agent to be a set of standard plugins; I think it might make sense to hoist handleSignals: false to the level of new ApolloServer().

[kubejm]

@glasser let me know if you'd like hand, I'd be happy to get involved and assist if it would be beneficial

[glasser]

@kubejm Thanks! I have an initial PR up at #4450. This is part of a series of work to pull a lot of super special cased code about talking to Apollo's commercial services out of ApolloServer and into normal ApolloServerPlugins.