-
-
Notifications
You must be signed in to change notification settings - Fork 340
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
Asynchronous Handlers #258
Comments
Update: Looking into the issue further, it seems that the individual threads can be blocked indefinitely. This means that Crow could be frozen for more time than timeout would normally allow, while keeping all connections assigned to the blocked thread on hold. |
Extra contextCurrently Crow's threading structure is as follows: The Worker threads are constantly running The Acceptor thread creates a new The bit most relevant to this issueThe Acceptor thread relies on Considering the fact that This becomes a problem when a handler (AKA Route) has to run any code that doesn't return instantly -due to being asynchronous- has to block the entire worker thread (and by extension the other connections). It is also a problem when considering the fact that any heavy operations will also need to run on (and block) the worker thread when they can be delegated to a separate thread and handled asynchronously. The proposed solutionMy suggestion would be for a worker to manage its connections.
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
The current discussion has only touched internals, but not what the actual goal is and what a handler will look like. Drogon uses a simple callback function and has support using c++20 coroutines with its own set of functions. Because using async requires some kind of runtime/executor/library and gluing two together is quite hard, I'd suggest also exposing the |
well to put the issue simply, if you ever have to spend 1 or 2 minutes spinning up a tape drive to get an obscure database record for 1 request, all the other requests in that pipeline will have to wait with no way for them to be executed while this 1 record is being retrieved. The general goal is to make it so that worker threads process requests as efficiently as possible, either by offloading the time consuming process and having the worker thread process other requests until it is complete. Or some other method (such as offloading the requests onto another worker thread that is free). I'm not exactly sure how having users schedule ASIO operations helps this since those operations still have to wait until the connection is deleted.. (I'm not saying this as a slight @dranikpg, I genuinely don't know and am asking) |
I surely get what the point of async is 😅 Let me be more specific. Most programming languages have some kind of executor that is used all over the ecosystem and that allows different libraries (webframeworks, database clients, etc.) to be used together. For Python - its asnycio, for Rust - Tokio, for JS - the Node runtime. C++ does not really have a single runtime and most frameworks invent something on their own. That is why drogon and oatpp have their own utilities for dealing with databases and redis. They can be used, for example, even with C++ 20 coroutines. Just because the handlers support asynchronous finishing, this doesn't mean any library/coroutine can be seamlessly glued on top of it. So the question is: what will asynchronous handlers look like?
How? A regular function cannot just suspend itself. How will the handler wake up again? The most viable and basic option is having a callback function. Like I mentioned, Drogon does it that way. CROW_ROUTE(app, "")
[](const crow::request, std::function<crow::response> readyFunc) {
// make async request to some other api
asio::async_read(some_stream, some_buffer, [](){
readyFunc(crow::response(buffer)); // tell crow the handler has a result ready
})
// the handler simply finished without a result - it will be provided via the callback
// so there is no blocking
} That will work with libraries that provide just callbacks, but is a bit more difficult with asio for example. The user needs its own io_context for all kinds of handler operations. |
The idea (at least in my head) was to use some form of coroutine to get back to where the function left off. The actual suspension and waking up were to be done by calling the async code through a wrapper function that changes the connection status and notifies the server to move on for the time being, once the code is done, a callback is run to change the connection status back.
Well the concern here would be how this approach affects everything that runs after the route handler returns. Since the current behavior is to simply start returning the response. Another point to look into would be how this would work with keepalive / timeout timers and if it's possible to keep the connection alive (without memory leaks) and still have the io_context handle different requests. |
I would have thought about tying the The handler function would have to use that I hope this was somewhat understandable. 😅 It should be possible to make that Example: // ...
CROW_ROUTE("/")([](crow::request req) {
auto t = std::thread([req]() -> void {
std::this_thread::sleep_for(std::chrono::seconds(2));
crow::finish(req, crow::response{"Work Done"});
});
t.detach();
}); Return-type |
But I guess this approach isn't all too far from just making every callback async, which would make it even easier to use as one can just block in the handler... This could work in a similar way. |
Sure, but that requires your end-user to use a specific coroutine in his handler. You can't do anything as soon as you hand control over to the handler. Boost and C++20 coroutines allow suspending, but you'd enforce them to be used. This wouldn't fit someone who wants to use a just custom library with callbacks.
Definitely better than passing an additional app around. Any thoughts on a completion function? One more option would be returning some kind of std::future, but one that would allow crow to listen on multiple of them. That'd make void handlers not that ambiguous and should maybe look more familiar. CROW_ROUTE("/")([](crow::request req) {
auto rsp = crow::Future{};
auto t = std::thread([req]() -> void {
std::this_thread::sleep_for(std::chrono::seconds(2));
fut = crow::response{"Work Done"}; // call assign operator that actually notifies
});
t.detach();
return rsp;
});
If you block, you're back to normal mode again 😅 Async handlers have to exit immediately (like in our examples). This requires two separate thread pools for blocking and non-blocking handlers. I'm being curios and asking this many questions, because I think about taking this up. So it'd be great if you'd share any more ideas/thoughs you have. |
Just to throw it in there: What about I think I'd prefer a mechanism similar to the |
std::promise is just the sender side of a future.
Thats the problem with them :/ We cant listen on multiple of them effectively. You can only block on one or loop over all of them. Calling crow::finish allows to directly handle the response.
That's true. It's just that someone who forgets to return a response from a handler unknowingly makes it an async one, which could be annoying to debug (like if you forget calling response.end). And there is nothing (no second argument, no return type) to remind or show via the signature, that this is an asynchronous handler. Besides this, most frameworks threat a void handler as an emptry response, that still has to be sent, so we won't be able to introduce in anytime later. (currently crow throws compilation errors on void handlers without a second response argument) |
Well, how about returning a specialized value like |
That'll work. This ensures: |
Can actual http response logic be contained inside of Like, crow::response res;
res.response_handler_ = [this, ...] (crow::response&& self) {
/* Send HTTP response using 'self' */
// Move crow::App's current crow::response handling logic inside of crow::response::response_handler_ callback.
// Since this may cause race condition between primary handler thread of crow::App,
// (Though I don't have any idea about internal implementations that well...)
// handling logic may be wrapped again with dispatch inside of provided end() handler.
// (asio::dispatch is used to prevent unnecessary event posting on synchronous invocation.)
// **example**.
asio::dispatch(this->worker_, [this, res = move(self)] { this->handle_response_(res); });
};
// Then when user or crow::App finishes response, it'll invalidate itself.
void crow::response::end() && {
/* do some end() work */
this->response_handler_(move(*this));
} Then user can simply move asio::thread_pool async_worker;
// In this way, app does not need to care whether response is async or not,
// thus user can route synchronous/asynchronous handlers in identical way.
CROW_ROUTE(app, "/some/test/api")(&handler);
void handler(crow::request const& req, crow::response& rep) {
asio::post(async_worker, [req, rep = move(rep)] {
/* do some time-consuming operation with 'rep' */
rep.end();
});
// Application instance becomes ready to accept next HTTP request immediately.
...
} Just curiosity :| |
+1 for the above. I think this suits most people and it's simple enough to understand, and it works well with threadpools. |
This wouldn't be too far from the solution @dranikpg and I worked out. I haven't yet looked at any code, including that of |
The actual response logic is already contained in Like this, for example 😅
The connection instance is alive until the request finished, you should not steal its data. Capturing by reference is fine. Maybe make response immovable 🤔 In case you want to return a So it seems there are a few issues with the |
@dranikpg It seems capturing reference and invoking 'end()' in another thread also blocks receiving next request. Is there any reference to implement receiving next request before current response is made? |
What is the progress of this feature? |
Does this feature is ready? |
on version 1.0+5, capture Crow::response in another thread and use response::end() to send http response to client then the crow threads will not block |
on version 1.0+5, this feature work well, but Aborted on version 1.1, any suggestions?
|
Dear @portsip i found the following info in gitter: Ex-maintainer The-EDev tried to implement it but could not make it work |
I found asynchronous handler crash in crow::response::end, because crow::response free in callback complete_request_handler_, so I modified crow_all.h L7457 like void end()
{
if (!completed_)
{
completed_ = true;
if (skip_body)
{
set_header("Content-Length", std::to_string(body.size()));
body = "";
manual_length_header = true;
}
if (auto handler = complete_request_handler_)
{
complete_request_handler_();
}
}
} This make asynchronous handler work again, so pls just try it.@InternalHigh,@witcherofthorns |
@shihzh hi, oouh, okay, i'll try this in the next free time and look at the source code in crow_all.h, thanks |
Hi folks, (read the thread again, and understand a little bit more, but...) I've been trying to understand these async discussions together with the generic documentation, but I am unable to understand just "to what level" supported asynchonous calls are at the moment (in master). I apologize if this is some form of ticket-hijacking, but I dont want to write a bugreport if I am simply using crow in an unsupported fashion. My CROW_ROUTE will often not finish with a crow::response.end(), but instead trigger another thread that will (eventually, using another async library call) do that.... and it works fine (afaik) with boost. If I've understood the above correctly, that should work? But now, I am in the process of moving to boost-less, using only the asio library, but if I do that, I will get 100% crashes in all my asynchronous calls. But since main crow page says that async is not (fully?) supported, I am wondering whether there is something more I have to do... Or is this a bug, when just using "pure" asio?
|
This is a comment made by u/aninteger on reddit
And he/she is correct. Crow does not have non blocking handlers. It would also be a nice feature to have, I'll have to look into it however since I don't even know where to begin working on this.
The text was updated successfully, but these errors were encountered: