@defer directive support

[bernardd]

I've been working on an implementation of the @defer directive (following up on this post: https://elixirforum.com/t/adding-defer-and-stream-to-absinthe/14546), and I'd appreciate it if you guys had time to give it a once-over just to make sure I'm not approaching it in an egregiously horrible way.

What's working so far:

• Functionality on scalar, object and list fields
• Deferring still works if the deferred field's value is already known
• Nested directives are ignored
• Fragment functionality
• Make the maximum time taken configurable

What still needs to be done:

• Probably a pile of stuff that I'm hoping you can help me enumerate :)

[collegeimprovements]

This could be a lifesaver for my project. Thanks a lot @bernardd for making it happen.

[bernardd]

@benwilson512 So I was thinking about our Slack discussion re continuation vs message passing. I now have it a bit clearer in my head why I went down the road I did: While calls can be made concurrent with multiple HTTP requests, that's not the way things are usually done with websockets (which is the connection style we will generally be dealing with here). My whole intention with this system was to allow for the client to continue to make and receive other requests while the DB (or whatever other backend) is churning away on a more expensive request. If we make the process block waiting for that request, which is what the continuation style would do (unless I misunderstand) then we're automatically precluded from handling anything else on the same websocket connection.

[bernardd]

So I think I've added everything I want to for now. I'm happy to for this to be reviewed with a view to merging what's there.

[benwilson512]

Unfortunately this can't work for two reasons. 1) The registry here is really just a copy of the Elixir 1.5 registry module, which we included in the code base simply to support Elixir 1.4. Elixir 1.4 didn't have the match_delete function.

2. The reason that the Elixir Registry module doesn't support passing in the pid is that unless the process is unsubscribing itself there are race conditions and the function is no longer safe.

[bernardd]

Dangit. Okay, I'll figure out something else.

[benwilson512]

If we make the process block waiting for that request, which is what the continuation style would do (unless I misunderstand) then we're automatically precluded from handling anything else on the same websocket connection.

Let me see if I understand what you're saying. Given the following doc:

{
  posts {
    title
    author @defer { name }
  }
}

What I was suggesting is that pass 1 on the document would execute essentially:

{
  posts {
    title
  }
}

and then when it was done push the result to the user. Then it would run the remaining blueprint which was:

    author @defer { name }

And push that. Now for a slightly different doc:

{
  posts {
    title
    author @defer { name }
    commentCount # this is slow
  }
}

Is your concern that the slowness of commentCount means that execution of the author field has to wait on commentCount to finish?

[bernardd]

Is your concern that the slowness of commentCount means that execution of the author field has to wait on commentCount to finish?

No, and sorry - I may be totally misunderstanding the internals here, but my concern is that if the user pushes another, unrelated query down the websocket, that second query will have to wait for all the pending @defers to be resolved before it can be handled. Similarly, if there are existing subscriptions on the socket, I'm concerned they will also be stalled while we wait for the @defer to be resolved. But I may have misunderstood how that stuff is handled.

[benwilson512]

Ah. Great question. Let's consider those two cases (queries / mutation vs subscriptions) separately cause they have different execution models.

Subscriptions

Let's start with subscriptions. Subscriptions are largely unaffected by this because subscription resolution does not happen inside the subscriber process, but rather inside the trigger process. If clients A and B submit subscription { newPost { title } } they are validated inside A and B's websocket process, but after validation they're placed in a centralized subscription document store (an ets table).

Then if C pushes a mutation, all relevant subscriptions are retrieved from the store and executed as a batch inside C's process, and then the results are pushed to A and B. The execution time of A and B's subscriptions does delay C's response, but this is an intentional design decision to create back pressure. If C's mutations are triggering a lot of work on the server then C needs to bear some of the cost of that work lest they overload the server.

Queries / Mutations

At the moment if you submit a query via websocket { posts { name } } this is indeed executed in a blocking way within the websocket itself, and then the response is sent. If the client sends 3 docs sequentially then they'll be executed sequentially, and this would apply to the implementation concept that I articulated. Execution of two documents containing @defer would not be interleaved or concurrent.

If concurrent document execution is the desired feature, I think it may be best to aim for that as its own feature within Absinthe.Phoenix, since this would solve that problem for not just @defer but also non deferred documents. The challenge is safety. We probably need some limit on the number of permitted "in flight" documents to avoid run away resource consumption.

[bernardd]

Okay, great, thanks - that all makes sense (except now I'm going to have to learn the guts of absinthe_phoenix too, dammit :) ). It also seems, at a glance, like this will solve the issue of needing an unsubscribe that takes a pid. I'll take a proper look at all this tomorrow, but thanks heaps for the feedback so far.

[bernardd]

Okay, take 2 reworked with continuation-style resolution. Enjoy :)

[bernardd]

Ping @benwilson512 Sorry to keep bugging you, but would it be possible to get some feedback on this? I'm not so worried about urgently merging it, but I have a couple of other use cases that would also benefit greatly from being able to use continuations like this and it'd be really good to know if it's likely to be generally acceptable in this form (even if there's issues with this specific PR). In particular, I'd like your opinion on the change to the run_result type and the Absinthe.Pipeline.continue/1 function. It turns out they'd probably also be very handy for the subscription-catchup system I was thinking about over in Slack.

[benwilson512]

Hey @bernardd I like the {:more, blueprint} result, I think that's a good way to go. The main reason I've delayed merging the PR is that I was hoping we'd be able to handle the defer directives with fewer explicit defer related code changes, instead leveraging existing capabilities.

I don't mind additional phases, since those could be added by anybody, but I was hoping there was a way to do this without adding a clause to the resolution phase. My thought would be that the schema author may need to add defer middleware to fields they want to allow being deferred, and that deferment middleware would be able to handle that instead of a dedicated phase.

One thing I wonder on the continuation struct: I wonder if it makes sense for it to include the pipeline that is necessary to do the continuing.

[bernardd]

Thanks for taking a look :)

Hey @bernardd I like the {:more, blueprint} result, I think that's a good way to go.

Great, because I want to use that for another thing too :) So I'll assume it's acceptable and look at making absinthe_phoenix work with it.

My thought would be that the schema author may need to add defer middleware to fields they want to allow being deferred, and that deferment middleware would be able to handle that instead of a dedicated phase.

It's probably an option; I'd need to look at it. The reason I didn't go down that road (and am still a bit disinclined to) was that, at least as far as the (very few) published details of @defer go, there's no special schema attribute that marks fields as "deferrable" or otherwise - same goes for @stream (which is likely to use a lot of this same code). So if we did go down that route, it would restrict the set of fields to which @defer or @stream could be applied without providing any introspection method to allow the client to see where it was allowed. It also feels a bit like putting implementation details ahead of functionality - I can't think of any good API-related reason why any given field(s) shouldn't be deferrable (of course equally I can't think of any good reason why a client would want to defer arbitrary fields...).

I was hoping there was a way to do this without adding a clause to the resolution phase

At a quick glance, it looks like it might be possible to simply insert an extra phase before Resolution which would strip out the deferred fields before they even hit that phase. Basically do what is being done for deferred fields in resolve_field/5 now, but in an extra, separate phase. Would that be reasonable if I could make it work?

One thing I wonder on the continuation struct: I wonder if it makes sense for it to include the pipeline that is necessary to do the continuing.

Quite probably. I'll take a look at that.

[benwilson512]

@bernardd OK, I see what you mean about whether we should require schema authors to annotate this stuff or not.

The main thrust of my point though was that I'm hoping we can avoid deferred_fields: [], streamed_fields: [], etc ... inside the execution struct, and instead push those values into the acc. I'm also still hopeful that we could manage to avoid adding additional resolution phase clauses. Extensive work has already been done to allow suspended fields and I'm not yet convinced that deferment or streaming needs a dedicated suspension mechanism.

[bernardd]

Okie dokes, try this version on for size.

I've managed to reduce the modifications to existing code pretty substantially. Apart from the addition of continuations, the only change of any note is the addition of a dynamic_middleware field to the document field structure. This seemed like the cleanest way of being able to programatically add middleware in a directive handler. (Otherwise, if you just add it to the field that's passed into the directive handler, it gets clobbered when the concrete field is retrieved).

Aside from those changes, the rest of the functionality is implemented by two extra phases and one new middleware.

[tlvenn]

@benwilson512 any chance to review and merge this PR ? It would be awesome to add support for @defer.

[benwilson512]

@tlvenn the plan at the moment is for me to finish the initial schema rework and then merge this in so we can include it in the 1.5 release. We should be days away from this.

[bernardd]

@benwilson512 I took the liberty of rebasing this against master. It should be good to go again.

[collegeimprovements]

Hi Guys, sorry to be a nag, but is it still happening in absinthe-1.5 ?

[tlvenn]

It is now targeted for 1.6 (See the milestone).

[binaryseed]

FYI this is getting picked up by the GraphQL working group. It looks like it's going to make it into the GraphQL spec eventually, so we'll definitely want to make sure we implement it according to where the spec evolves...

https://github.com/graphql/graphql-spec/blob/master/rfcs/DeferStream.md

[sethwebster]

Any news on this one?

[benwilson512]

This will be revisited after the Absinthe 1.5 release later today.

[binaryseed]

Also, the GraphQL spec for this is still being discussed, I'd suggest watching the RFC document I linked above...

[benwilson512]

I think the right call in this case is to close the PR and await clarity from the spec.