8000 Implement promises via CompletableFuture by akhahaha · Pull Request #202 · graphql-java/graphql-java · GitHub
[go: up one dir, main page]
Skip to content

Implement promises via CompletableFuture#202

Closed
akhahaha wants to merge 0 commit intographql-java:masterfrom
akhahaha:master
Closed

Implement promises via CompletableFuture#202
akhahaha wants to merge 0 commit intographql-java:masterfrom
akhahaha:master

Conversation

@akhahaha
Copy link
@akhahaha akhahaha commented Sep 9, 2016 •
edited
Loading

Related to Asynchronous DataFetcher request (#37)

Massive change that deprecates nearly all key interfaces and moves the SDK requirement to 1.8. Should be considered for a 3.0.0 release.

Error handling and propagation was a little tricky and could probably use some refinement by someone more talented than I, but all tests seem to pass.

Special thanks to @hepin1989 for his input on using CompletableFutures.

@oexza
Copy link
oexza commented Sep 9, 2016

Hello @akhahaha nice work, I think its better to return CompletionStage rather than CompletableFuture, since CompletableFuture is an implemention of CompletionStage, like its done in Akka and PlayFramework which use this Interface extensively for asyncronous operatio 8000 ns. Thanks.

@dminkovsky
Copy link
Contributor
dminkovsky commented Sep 9, 2016

Hi @oexza. Can you please provide your thoughts regarding #37 (comment) and on?

@He-Pin
Copy link
He-Pin commented Sep 9, 2016

@oexza in fact I like the syntax of

return env -> CompletableFuture

@akhahaha
Copy link
Author
akhahaha commented Sep 9, 2016 •
edited
Loading

@oexza @hepin1989 Lol damn, well I just changed it to CompletionStage, but I can always revert it. What are the pros and cons?

@dminkovsky
Copy link
Contributor
dminkovsky commented Sep 11, 2016

@akhahaha thanks again for your work on this PR. Following the discussion on #37, I think it does make sense to close it for now.

If you see any other aspects of this project that you could improve—especially docs are you mentioned on #37—please do. We are chatting on Gitter.

@dminkovsky dminkovsky closed this Sep 11, 2016
@dminkovsky
Copy link
Contributor
dminkovsky commented Sep 18, 2016

I think this makes sense to do as a sub project, if anyone would like to do that.

@dminkovsky dminkovsky reopened this Sep 18, 2016
@akhahaha
Copy link
Author
akhahaha commented Sep 19, 2016

What would that entail specifically?

@dminkovsky
Copy link
Contributor
dminkovsky commented Sep 20, 2016

@akhahaha I'm not sure! I would like to explore concrete use-cases. For example, @pcarrier wrote the other day

@dminkovsky for example to run (strictly ordered) operations on netty's event loop
@dminkovsky including non-blocking I/O

I'd like see a code sample that shows this usage, so we can figure out how it might be targeted. If you're trying to get each field (and ultimately a whole query) resolved on an event loop, then those tasks need to somehow get on to that event loop. An appropriate execution strategy needs to somehow take this in to account.

I've been Googling around for some pure Java examples of async request processing. Some things I've found:

  • Dropwizard async example: uses the AsyncContext API to asynchronously handle the request. AsyncContext#start() still delegates to an underlying pool of threads. An article that discusses this API uses an explicitly declared thread pool.
  • Jersey async documentation: like the Dropwizard example, the crux of this asynchronicity is suspending/resuming connections so that threads can be used for processing instead of maintaining idle connections.

I've not explored Netty and am hoping @pcarrier will provide his use-case.

http://www.nurkiewicz.com/2015/11/which-thread-executes.html
https://www.mailinator.com/tymaPaulMultithreaded.pdf

@akhahaha
Copy link
Author
akhahaha commented Sep 20, 2016 •
edited
Loading

@dminkovsky It sounds like @pcarrier might be having similar "issues" from what we discussed in #37. Have you shown him that thread and perhaps this PR?

To reiterate, to accomplish the async functionality that I think and he and I were looking for, simply use blocking DataFetchers and then perform the serial execution using the ExecutorService strategy on another thread using an async wrapper of your choice. Each DataFetcher will then resolve in parallel, and the async wrapper should return when all have been resolved.

It's not intuitively async like we'd expect, but the result should be equivalent. I guess it does beg the discussion again of whether or not async functionality like this should be built-in by default.

@dminkovsky
Copy link
Contributor
dminkovsky commented Sep 21, 2016

@akhahaha thanks for you response.

Have you shown him that thread and perhaps this PR?

I @-tagged him in my post above, and just in case just highlighted him in the Gitter chat with a link here.

To reiterate, to accomplish the async functionality that I think and he and I were looking for, simply use blocking DataFetchers and then perform the serial execution using the ExecutorService

Yeah, that seems to be the Java/JVM way. But for some reason this topic keeps coming back up, in issues and in the Gitter chat. People seem to want to minimize blocking and context switching. My Google searching suggested that this approach might not actually yield performance gains, but this has come up time and time again. I personally like the simple code style that comes with blocking threads, and appreciate that Dropwizard claims 30-50k reqs/seq with a default 1024-count thread pool. Then again, maybe people can achieve even more on a tight event loop?

It's not intuitively async like we'd expect, but the result should be equivalent. I guess it does beg the discussion again of whether or not async functionality like this should be built-in by default.

I think so too. But given the repeated interest I think I closed this PR too hastily. I would like to keep this main project on Java 1.6 to facilitate "legacy" users experimenting with GraphQL on Java. But at the same time perhaps a motivated, interested party will be interested in doing a sub-project.

@He-Pin
Copy link
He-Pin commented Sep 21, 2016

async is for not blocking on IO,not for speed.

@akhahaha akhahaha closed this Sep 22, 2016
@akhahaha
Copy link
Author
akhahaha commented Sep 22, 2016

I'm reworking this

@dminkovsky
Copy link
Contributor
dminkovsky commented Sep 22, 2016

Hi Alan,

Cool, sounds good.

In terms of getting this incorporated into this project or built out as a
subproject, I think it would make sense for people to discuss further to
target particular use cases.

On Thu, Sep 22, 2016 at 1:11 AM, Alan Kha notifications@github.com wrote:

I'm reworking this

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#202 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANWZfJxTkIFho32zyH13M7jazmVMgpXks5qsg3xgaJpZM4J4qKX
.

@akhahaha
Copy link
Author
akhahaha commented Sep 22, 2016 • 8000
edited
Loading

I think I'm onto a fairly simple solution right now. I've extended DataFetcher into an AsyncDataFetcher abstract class.

import java8.util.concurrent.CompletableFuture;

import java.util.concurrent.ExecutionException;

public abstract class AsyncDataFetcher implements DataFetcher {
    public Object get(DataFetchingEnvironment environment) {
        try {
            return getDataFuture(environment).get();
            // TODO Handle exceptions
        } catch (InterruptedException e) {
            e.printStackTrace();
        } catch (ExecutionException e) {
            e.printStackTrace();
        }

        return null;
    }

    public abstract CompletableFuture<Object> getDataPromise(DataFetchingEnvironment environment);
}

Users can then return their data in a promise using getDataPromise(). This particular CompletableFuture class uses the streamsupport-cfuture backport library, which should be compatible at least back to 1.6, although I'm not entirely sure if it's interchangable with the native 1.8 versions.

I think there should also be GraphQLAsync class that wraps the default GraphQL in an async wrapper (via callbacks or more CompletableFutures). It should also use an ExecutorService by default in order to achieve the parallel completion necessary for equivalent async performance. I'm not too familiar with the pattern, so I'm not sure if we can provided a default ExecutorService implementation or if users are supposed to provide their own.

Unlike my previous implementation, this method should not require changing any other part of the library.

I'd recommend this be placed into a subproject/module (requiring current graphql-java to be moved into a "core" module). It shouldn't be too hard to maintain this plugin alongside the main project.

Thoughts?

@dminkovsky
Copy link
Contributor
dminkovsky commented Sep 23, 2016

@akhahaha that is pretty interesting. I thought about this yesterday and here are my thoughts:

This particular CompletableFuture class uses the streamsupport-cfuture backport library, which should be compatible at least back to 1.6, although I'm not entirely sure if it's interchangable with the native 1.8 versions.

I was thinking this could be a subproject and therefore just simply target 1.8. I think the async/non-blocking crowd is all using 1.8.

I think there should also be GraphQLAsync class that wraps the default GraphQL in an async wrapper (via callbacks or more CompletableFutures). It should also use an ExecutorService by default in order to achieve the parallel completion necessary for equivalent async performance. I'm not too familiar with the pattern, so I'm not sure if we can provided a default ExecutorService implementation or if users are supposed to provide their own.

I'm not sure what to say about this because I don't know how this would integrate with the bigger picture for people who are doing async/non-blocking request handling already. These users already have a way they're handling concurrency, and I suspect they want this to integrate nicely with the way of handling concurrency. Take for example the RxJava README. There they mention as one of its features:

  • Non-opinionated about source of concurrency (threads, pools, event loops, fibers, actors, etc)

I wonder how they do this... If we have a slew of CompletableFutures that run on the default runtime pool, async people will definitely not want that. I suspect giving users the option to specify their own ExecutorService won't be what they're looking for either, at least not entirely. I think users will want fields to get resolved within their existing source of concurrency.

So, with all that said, I am not sure how to proceed. It's almost like there's no generic solution to be had? I want to further study the Servlet 3.0 asynchronous handling API and Netty. I think an answer, if there is one, might be found somewhere around there.

Please let me know if any of this is unclear or needs more elaboration. Thank you.

PS. Please see also 8 Asynchronous & Non Blocking

@akhahaha
Copy link
Author
akhahaha commented Sep 24, 2016 •
edited
Loading

Non-opinionated about source of concurrency (threads, pools, event loops, fibers, actors, etc)

I think my proposed solution is in fact non-opinionated about the source of concurrency. From what I can tell, both CompletableFuture and Observable are simply fancy callback/runnable handlers. They do not themselves run on another thread.

Some sort of ExecutorService is necessary to achieve the same functionality like we discussed. I agree that it does seem undesirable, but the other alternative is to follow the guidelines listed in your Ratpack link, namely:

"Request handling is organised as a pipeline of asynchronous functions."

Incidentally, I believe this is how my original PR worked, by creating a pipeline of CompletableFutures from DataFetchers to ExecutionResult. However, I don't believe it would be possible to make this a subproject, nor should we maintain it as a sister project. If we are to use this method, I believe this should be become the default implementation.

@dminkovsky dminkovsky mentioned this pull request Dec 20, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@akhahaha @oexza @dminkovsky @He-Pin
0