perwendel / spark

A simple expressive web framework for java. Spark has a kotlin DSL https://github.com/perwendel/spark-kotlin
Apache License 2.0
9.64k stars 1.56k forks source link

Support of event based (non-blocking) request processing. #549

Open TarlanT opened 8 years ago

TarlanT commented 8 years ago

Currently, for request processing, SparkJava completely relies on HTTP thread pool of Jetty (by default 8 - 200 threads). Which in it's own turn is non-blocking on networking end, but it's blocking on request processing (business logic) side (Handlers/Filters/Matchers...). Any blocking (I/O bound, JDBC etc) operation has a potential to exhaust the Jetty's HTTP thread pool. In that sense, Spark currently is not leveraging existing asynchronous Servlet 3.1 implementation by Jetty. To increase performance potential of Spark, framework needs to add support of event based (non-blocking) processing based on it's own thread pool. Which is currently easily achievable with a combination of Servlet 3.1 + Java 8 CompletableFuture API.
With this combination there is no need for integration with high level Akka or RX frameworks.

Following is a sample code that can actually achieve the goal stated above:

/**
 * Simple Jetty Handler
 *
 * @author Per Wendel
 */
public class JettyHandler extends SessionHandler {

    private Filter filter;
    public JettyHandler(Filter filter) {
        this.filter = filter;
    }

    @Override
    public void doHandle(
            String target,
            Request baseRequest,
            HttpServletRequest request,
            HttpServletResponse response) throws IOException, ServletException {

        if (NOT_ASYNCH) {
            filter.doFilter(wrapper, response, null);
        } else {
            AsyncContext asyncContext = wrapper.startAsync();
            asyncContext.setTimeout(60000);
            CompletableFuture.runAsync(() -> {
                try {
                    filter.doFilter(wrapper, response, null);
                }
                catch (IOException | ServletException ex) {
                    throw new RuntimeException(ex);
                }
            }, executor)
            .thenAccept((Void) -> {
                baseRequest.setHandled(!wrapper.notConsumed());
                asyncContext.complete();
            });
        }
    }
}

The SPARKS_OWN_ASYNC_EXECUTOR above may look like this:

private static final ThreadPoolExecutor executor = new ThreadPoolExecutor(200, 200, 60, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>());
    static { executor.allowCoreThreadTimeOut(true); }

If you run a benchmark with significant amount of simultaneously active client sockets, and monitor threads of the active application with change above. You'll see that Jetty will create multiple times less of HTTP threads (you can identify them by "qtp" prefix) than it typically does. An those created will be nicely alternating between being busy and parked. Instead, there will be Spark's own thread pool created witch will have all threads being 100% busy under high load. Which is the exactly the goal with event based approach. And in case on of Sparks own thread to block, then it will not degrade the performance of Jetty's performance.

The idea is to cut the reliance on Jetty's HTTP thread pool. And not force Spark users do following:

public static void main(String[] args) {
    get("/benchmark", (request, response) -> {
        AsyncContext ac = request.raw().startAsync();
        CompletableFuture<Void> cf = CompletableFuture.supplyAsync(() -> getMeSomethingBlocking());
        cf.thenAccept((Void) -> ac.complete());
        return "ok";
    });
}

(This comment was edited by @tipsy, to fix formatting issues)

Zavster commented 8 years ago

Alternatively you could just at a method on the Spark request object called async() that takes in your Callable that performs the long running task. Whatever is produced by the Callable is what gets sent back to the caller.

public static void main(String[] args) {
    get("/benchmark", (request, response) -> {
        request.async(() -> getMeSomethingBlocking())
    });
}
ruurd commented 8 years ago

NOT AGAIN! Asynchronous Spark has already been beaten to death in another issue. -1500456 :(

Lewiscowles1986 commented 7 years ago

@Zavster tried your code, it doesn't seem to work and looking at the code there is no async method for the request.

@ruurd links to other issues covering?

ruurd commented 7 years ago

208 - spark is simple and blocking. that is why it is simple to use.

Lewiscowles1986 commented 7 years ago

should probably close this then and mark as wontfix. I fully appreciate the effort the maintainer has gone to for me to ask for features is cheeky AF so apologies. ~What I cannot work out is how hello world on an i7 using 8 threads is not serving > 1000/req sec, thought blocking might be loosing tiny fractions that would aggregate~ Turns out launching with -Xmx=4096M makes it faster anyway :laughing:

Lewiscowles1986 commented 7 years ago

Just in-case anyone is wondering why I now think this does not need async

siege -c 500 -b -t 1m http://localhost:3000
** SIEGE 3.0.8
** Preparing 500 concurrent users for battle.
The server is now under siege...
Lifting the server siege...      done.

Transactions:            1339190 hits
Availability:             100.00 %
Elapsed time:              59.94 secs
Data transferred:          14.05 MB
Response time:              0.02 secs
Transaction rate:       22342.18 trans/sec
Throughput:             0.23 MB/sec
Concurrency:              496.64
Successful transactions:     1339190
Failed transactions:               0
Longest transaction:           15.04
Shortest transaction:           0.00

the program

import static spark.Spark.*;

public class HelloWorld {
    public static void main(String[] args) {

        port(3000);

        int maxThreads = 8;
        int minThreads = 2;
        int timeOutMillis = 30000;
        threadPool(maxThreads, minThreads, timeOutMillis);

        get("/", (request, response) -> {
            return "Hello World";
        });

        awaitInitialization();
    }
}

launch command

java -jar target/hello-1.0-SNAPSHOT-jar-with-dependencies.jar -Xmx=4096M
mlengle commented 7 years ago

Helloworld controllers does not need async, but any service with big internal latency and a noticeable load surely can benefit from it. For our company it's major stopper for migration from dropwizard (it's based on Jersey and offers async controllers).

Lewiscowles1986 commented 7 years ago

@mlengle the point was not you should use a HelloWorld controller :roll_eyes:. I feel you must know that is not what I meant, but that compared to other frameworks silly benchmarks (because at the framework level that's all you have, and the benchmarks all pretty much to hello-world.) 22,342 transactions per second shows how fast and lightweight the framework is.

Do you have any numbers reflecting that Spark specifically would encounter an increase in speed or requests per {period} by using async? Because I don't so I was working with what I did have access to. I'd also suggest that dropwizard being opinionated probably gives it not bad speed (also consider it was running on a laptop that is running a VM in the background and coming for 3 years old)...

mlengle commented 7 years ago

@Lewiscowles1986 it's not about requests per second, there's good example in the Jetty docs: https://www.eclipse.org/jetty/documentation/9.3.x/continuations.html#_asynchronous_restful_web_service In our practical case both service load and latencies were even higher and so was the gain from the async model.

Lewiscowles1986 commented 7 years ago

Thanks I'll try to digest and understand...

ruurd commented 7 years ago

@mlengle so basically you let async IO fix the problems you have in your software.

mlengle commented 7 years ago

@ruurd nice trolling, but no. and reread the link I've sent, it's not about async io

ruurd commented 7 years ago

@mlengle drats. caught. "service with big internal latency" is the red flag for me. We know up front that we can't take too long to service a request. But still we do. And a lot of times we just throw more stuff at it instead of having a good look at the stuff we wrote and actually come up with a proper solution. Hence the trolling...

Lewiscowles1986 commented 7 years ago

@ruurd whilst I don't understand even after reading the article several times it may be @mlengle knows something we don't, lets not descend into criticism of his or his teams programming on a friday please

mlengle commented 7 years ago

@ruurd @Lewiscowles1986 There's Play framework heavily built upon async principles, check it if you're curious. There're things like streaming and websockets which create long living connections by their definition. And yeah, there're long latency services, sometimes because of a bad architecture, sometimes because of huge amount of data to process, or heavy load with limited hardware resources and the storage systems that may have their own failures, that's reality and web frameworks have to deal with real world problems, not with the perfectly polished backend code. And we're talking here not about some ugly hack, but about a popular principle in modern software development: "don't block"

wpc009 commented 7 years ago

I think @mlengle is right. no modern software should be blocking this is the whole point. Blocking is simple, but some kind of a waste. All of us agree blocking should be avoided if you want to grow big.

ruurd commented 7 years ago

Not all software we create is intended to grow big. Why waste energy in unblocking a piece of software that is blocking to begin with and then use it for stuff that does not grow big? Now THAT is a waste. Use spark for what it is good at. If you grow big you have time to switch to something that fits your future needs. Only using tools and libraries that are geared towards big-bigger-biggest is lunacy. Why would I want to gear up a bunch of replicated hardware with all the difficulties that brings with it for a system that only needs to process let's say 10 requests per minute? Blocking IO in that case is a much easier to understand model and Spark is covering that excellently. Keep in mind that a lot of systems are small and that only the very lucky few will grow big. And those that do will go through lots of increments and lots of changes in the tooling.

wpc009 commented 7 years ago

It's not about growing big. non-blocking io makes us using the hardware more efficient. It reduces the hardware idle time, which results in less total execution time.

ruurd commented 7 years ago

Poppycock. It makes spark less simple thus more difficult to extend and maintain and document and more difficult to use. For what? For making more efficient use of hardware? That simple little server that is running your simple little spark program that serves that little amount of transactions? In recent servers the bottleneck often is disk IO and networking, not processing. Faster processing will just result in the CPU spending more time idling. Don't waste your money on hacking away on a simple blocking product like Spark. If you really need a framework that is nonblocking - and trust me, you don't - then first look for the simplest way to scale out. And if that avenue has been exhausted then the time has come to look for a framework that is built on async IO.

phongphan commented 7 years ago

I encountered this thread while researching the framework. I understand that the spark developer would trade it for the ease of usage (not suitable for some cases, but good selling point).

What I don't understand is why you let ppl like @ruurd, who clearly doesn't understand how and why asynchronous processing could mitigate IO latency, kept trolling almost every single issue about async support.

Frankly, I was though that @ruurd is one of spark maintainers (which is unbelievable)

Read above and #208 then judge it yourself

tipsy commented 7 years ago

@phongphan I disagree about @ruurd trolling and that he "clearly doesn't understand". His posts are a bit abrasive and exaggerated, but he makes some good points, and he understands the intention behind Spark. We're considering async for v3, but we'll only do it if we can maintain the simplicity Spark currently has.

ruurd commented 7 years ago

@phongphan you realise that this is a public place do you? I think your comments are rather unfriendly and ad hominem. I know async can mitigate IO latency but I also know that having to mitigate IO latency is not a concern when you are just starting off with a backend service. That is the prime reason to pick Spark: the concept is simple and synchronous and the time you need to go from zilch to working is short. Just have a look at a different issue where someone asked for rerouting requests. @tipsy gave an excellent solution for it however that particular user already moved on to a different framework HOWEVER spark already performed its service for that user. I'm with @tipsy on async Spark. It is a good move to make Spark async provided it keeps the major selling point it has at the moment: simple to use - simple to understand - simple to implement.

tipsy commented 7 years ago

@TarlanT I tried plugging in your code, but I didn't notice any performance gains. Jetty did create a lot fewer threads, but in return Spark created a lot more threads. Just seems like we're shifting threads from one pool to another 🤔

mlengle commented 7 years ago

@tipsy Cause this is completely wrong way to the non-blocking web layer :) The right way with jetty is mentioned in the docs https://wiki.eclipse.org/Jetty/Feature/Continuations#Using_Continuations

so, we should have separate async handler which suspends continuation, then async-supporting controller code should return CompletionStage, and we add processing stage like this:

returnedCompletionStage 
.thenApply(returnedResponse -> {/* code to pass the response */  contunuation.resume()})
.exceptionally(e -> { /* appropriate err handling */});
ruurd commented 7 years ago

Sigh.... @tipsy kill it. kill it with fire.

TarlanT commented 7 years ago

@tipsy Hmm... interesting. How did you test the performance? I myself couldn't test it properly. Did not have proper dedicated server and clients.

As per shifting threads between pools - yep. CompletableFuture submits tasks to JDK's built-in ForkJoin thread pool. Which is axacly what's needed. There should be less threads than in Jetty. And threads in ForkJoin being mostly busy is a good sign.

BTW. ForkJoinPool uses Work Stealing scheduling technique. That's according to many reasearch papers and benchmarks have better performsnve in a wide range of cases. Since there is no shared queue to mutex/lock on.

TarlanT commented 7 years ago

@mlengle Cool. Sounds like a Jetty way of doing the Async. But I don't think it's the root cause of what @tipsy saw. At the end of the day, it just comes down to submitting a task/work to some queue/pool and unblocking the calling thread.

tipsy commented 7 years ago

Sigh.... @tipsy kill it. kill it with fire.

Don't worry, we're not rushing into anything. I don't have a lot of experience with Java async, so I'm just investigating a little. We won't implement anything that will inconvenience anyone.

tipsy commented 7 years ago

@TarlanT but you're not actually doing anything async? In your code you say "Hey Jetty, this thing is async, I'll let you know when it's done", then you run doFilter in a separate thread, but doFilter is still blocking Then you say "Hey Jetty, that thing is done." The work will take the same amount of time (or slightly longer because of the overhead). Wouldn't it be better to just increase jetty's threadpool?

TarlanT commented 7 years ago

@tipsy Sure. You right. But still. Isn't it better to have Jetty http threads always available for any request, and have blocking calls queued up in WorkJoinPool queues? Rather than let Jetty create new thread or retrieve one from much larger precreated pool? Just creating more threads for any blocking call is the issue that may lead to thread pool exhaustion. More threads not always good. Actually it's bad most of the time. Not always though.

tipsy commented 7 years ago

Isn't it better to have Jetty http threads always available for any request, and have blocking calls queued up in WorkJoinPool queues?

I don't know enough about Java threading to answer that. I thought what you said made a lot of sense at first, but I couldn't setup any scenarios in which it gave better performance.

With your approach the custom pool would be at 100% thread activity and requests would start lagging, while Jetty stayed at 14 threads (186 idle threads)

tipsy commented 7 years ago

@Zavster's suggestion for request.async(() -> getMeSomethingBlocking()) is very easy to implement, server independent, and actually async. If we do async I think we should do that, and I don't even think @ruurd would complain about it.

mlengle commented 7 years ago

Guys, to get the benefits from non-blocking controller you need to have non-blocking service code (returning CompletableFuture, RxJava's Observable, Scala futures, etc). If your service relies on blocking DB driver library or other similar blocking resource BUT still spends noticeable amount of cpu -- it makes sense to make the service non-blocking and then use non-blocking controller. (also nowdays we have some choice of non-blocking jdbc drivers and many nosql solutions). In this case neither jetty threads nor execution context threads will block waiting and be reused for serving other requests.

On 5 June 2017 at 21:31, David notifications@github.com wrote:

@Zavster https://github.com/zavster's suggestion for request.async(() -> getMeSomethingBlocking()) is very easy to implement, server independent, and actually async. If we do async I think we should do that, and I don't even think @ruurd https://github.com/ruurd would complain about it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/perwendel/spark/issues/549#issuecomment-306279876, or mute the thread https://github.com/notifications/unsubscribe-auth/ABsGCuSaBpyi4n71d0JT8SSbneq_0OuUks5sBFd0gaJpZM4IZx8q .

-- bye, Gleb.

tipsy commented 7 years ago

@mlengle Yes, but that could be solved with something like:

@FunctionalInterface
public interface AsyncHandler {
    CompletableFuture<Void> handle();
}

public void async(AsyncHandler asyncHandler) {
    AsyncContext asyncContext = servletRequest.startAsync();
    asyncHandler.handle()
        .thenAccept((Void) -> asyncContext.complete())
        .exceptionally(e -> {
            asyncContext.complete();
            throw new RuntimeException(e);
        });
}

It's not a very pretty solution, but it works, and it's not an important feature (imo).

mj1618 commented 6 years ago

I think it's good to have the option to respond to a request asynchronously. I don't think it has to compromise the simplicity of the current blocking features.

Simply allowing the async() method will allow people to use libraries that provide callbacks instead of blocking for IO. In this case I couldn't find a way to make Spark allow the response to happen on this callback. startAsync() doesn't seem to work for me, the output stream seems to get closed anyway.

I think it's an important feature as more and more java libraries out there are moving towards non-blocking IO, and by making this feature optional to Spark users means the framework has more flexibility. And as long as it's an optional one I don't see how it would make anything more complicated.

Tbh I don't see the point of another Thread pool for this, that doesn't seem necessary to accomplish async.

Is this something you're keen to have? If so I don't mind doing some work on it.

ruurd commented 6 years ago

You know what? I give up.

tipsy commented 6 years ago

Is this something you're keen to have? If so I don't mind doing some work on it.

Only if you can make it work without bothering @ruurd 😁

I know that seems hard since he's bothered by people even discussing it, but if you manage to introduce it to the framework in a way that non-async people don't notice it, I think it should be included.

ruurd commented 6 years ago

Mhrhmrhm. I think I have a couple of good arguments. But somehow every once in a while somebody comes along and starts this thing all over again from the start.

mj1618 commented 6 years ago

hey @ruurd yea I read your comments - you make some good points and I really respect your opinion.

Spark is simple and lets keep it that way - 100% agree with you on that, I think most people do. And given most Java libraries are sync + blocking (e.g. jdbc protocol itself is sync blocking) most request handlers are going to be the same. However it does seem the community is keen to at least have the option of async on the odd occasion it comes in handy.

For me at least, I'd like to have the option to use an async library in my request handlers. E.g. an async http client, or a call to a streaming application such as kafka.

The reason is not to offload to another thread pool - I agree thread shifting is just unnecessary overhead. But for truly non-blocking calls (similar to what NIO provides) it can be better to not chew up a thread waiting for bits to go over the wire.

But considering lean principles I think it's prudent to introduce only the most basic capability though - and to keep the default sync + blocking. Then we can see how the community responds - and see if it actually does open up any opportunities. If not - you can always remove it! A lot of great open source projects remove features that made sense at the time but turned out to be unnecessary, or were surpassed - e.g. GC in Rust.

One downside to request.async() is that there is still a required return value - so this might be confusing when doing async. Express gets around this by having the response passed to the response object and not having a return value. But that would change the protocol here.

How about we just handle the return value being a Future? That way it would change nothing about the current interfaces, so to the average sync+blocking handlers it would make no difference. And when we see a Future as the return value, we async the request and respond on the future completing?

If it's ok with you @ruurd I'll code up some sample code for you and @tipsy to review.

ruurd commented 6 years ago

I see two roads ahead here. Either we fork spark and turn the fork into one that uses async IO or we venture into an architecture where we can plug in sync or async or both backends. This would probably mean that the project has to seriously adapt the interface we are using.

So. In order not to up the ante too much: let's see if it is possible to add in async with minimal changes in the higher level way of using spark. Yes. I know. Quite a radical departure of 'let us keep spark lean, mean and simple' but convince me. Maybe even Java9 can be of service in this venture.

mj1618 commented 6 years ago

Hey guys - just had a go at a basic implementation, just so we've got something concrete to look at. I know this isn't good to merge and will get plenty of feedback, but at least it's something to chat about.

https://github.com/perwendel/spark/pull/953

Basically if you return a CompletableFuture, then it will make the request async. Otherwise everything remains the same as before.

I did break up the two quite long functions, though I understand if this is too much change and I'd be happy to rewrite it based on your comments.

Thoughts?

mj1618 commented 6 years ago

@ruurd or @tipsy do you have any initial feedback or thoughts?

This is an example that makes use of the API for reading a file for example - https://github.com/perwendel/spark/pull/953/files#diff-fde50b24686da7d9cfc2d49281002c0bR55 It's possible this might suffer more from the context switching than it will gain from the async. Might be worth checking this?

Another thing I can do is show a demo of where we could avoid all the threads being chewed up during a blocking task, and show how an async handler could avoid this. Non-blocking would be a good solution in this use case.

tipsy commented 6 years ago

I haven't had time to look through it properly, but my first impression from skimming through the code was good. The internal change seems quite big, and it looks like some refactoring is needed. I'll have a closer look next year (holidays soon). Ultimately it's not up to me though.

mj1618 commented 6 years ago

Sure sure no problem, have a good holiday.

Cheers, yea I did feel like I was making too many changes here. Keen to hear how you’d like to see it done - maybe a little refactoring of those long fns first.

ruurd commented 6 years ago

Hehheh... I also have to find some time to do that. After doing my 8-hour shift at work.

mj1618 commented 6 years ago

Yea understand no problem guys, take your time!

mj1618 commented 6 years ago

Hey guys thought I'd follow up and see if you'd like me to fix up my messy PR a bit?

mj1618 commented 6 years ago

If so keen to get some feedback and suggestions first

joatmon commented 6 years ago

Hi,

My team has been using Spark as a platform to build microservices for the last year. We have one service that is still running on Spring Boot that we would like to port to Spark, but it makes heavy use of Asynchronous Requests. We've reviewed the PR from mj1618, and don't think that it would work for our use case because we also need the ability to stream back very very large responses.

So we have come up with an alternative PR that we have tested with our application (at transaction rates of up to 3000 requests per second)

TL;DR

This PR:

USE CASE

I'll use our service as an example of why asyncronous request handling is needed. Our service basically exposes a set of REST API's that can be used to issue queries against any of several backend databases. The user of the REST API can submit any query they wish, so we have no idea how long the query will run or how much data it will return. It could run for 1ms and return 1 row of data or it could run for 2 hours and return a million rows of data.

In typical syncronous request processing, these queries would be running in the Jetty request thread. So if enough long running queries are submitted, we will run out of request threads. 🙁

So instead we use asyncronous requests which free up the Jetty request thread once we have put the request on an internal queue. Then we have our own thread pool that executes database queries from the internal queue and then streams the results back to the client.

Because we are potentially returning so much data, we cannot keep all of the response body in memory at one time. So we need to be able to stream the response directly to the output stream of the HTTP response. This is the part that doesn't work well with the existing Spark API's because they store the response body in string.

EXAMPLE: Spark API's

Here is a simple example of a Spark Controller method that creates its own thread to use to process the request asyncronously:

        get("/async", (req, res) -> {
            final AsyncContext ctx = req.startAsync();
            Thread t = new Thread(() -> {
                try {
                    Thread.sleep(500);
                    res.body("Hello from Async!");
                    ctx.complete();
                }
                catch (Exception e) {
                    e.printStackTrace();
                }
            });
            t.start();
            return "";
        });

EXAMPLE: Servlet 3.0 API's

Here is the same example implemented with Servlet 3.0 API's.

        get("/traditional", (req, res) -> {
            final AsyncContext ctx = req.raw().startAsync();
            Thread t = new Thread(() -> {
                try {
                    Thread.sleep(500);
                    PrintWriter writer = ctx.getResponse().getWriter();
                    writer.write("lots of data");
                    writer.close();
                    ctx.complete();
                }
                catch (Exception e) {
                    e.printStackTrace();
                }
            });
            t.start();
            return "";
        });

Both of these approaches are supported by this PR.

kboom commented 6 years ago

In fact, if you say on the front page that spark can be used as a great replacement for node, you should provide this functionality... otherwise it's not entirely true, is it?