Closed mdaneri closed 1 month ago
Hey! I managed to get time to review π
I would recommend raising an Issue first for larger feature work, so it can be discussed before diving into the solution π
If I'm right, this is a wrapper for Routes, which sets the Route logic as an async task with optional routes for info retrieval?
If so, I'd recommend the following:
Set-PodeTaskConcurrency
is available to increase the amount used.
Async.ps1
file keep the function naming consistent like with other files, for example:
Add-PodeQueryTaskRoute
> Add-PodeAsyncQueryRoute
Add-PodeStopTaskRoute
> Add-PodeAsyncStopRoute
Add-PodeGetTaskRoute
> Add-PodeAsyncGetRoute
Set-PodeRouteAsync
> Set-PodeAsyncRoute
Remove-PodeAsyncRoute
which calls Remove-PodeRoute
, but also Remove-PodeTask
- moving any runspace clean-up into here.Set-PodeAsyncRoute
it might be an idea to allow a "WebhookUrl" to be supplied, so when the Task the Route invokes completes it could send a callout to a webhook.Get-PodeUserRequest
, there's already Get-PodeHeader
, so I'd just split these up as Get-PodeData
, Get-PodeQuery
, and Get-PodeParameter
each with a -Name
parameter. Placing them into the public Utilities.ps1
file.Regarding the Get-PodeData, Get-PodeQuery, etc., I agree with your suggestion. I'll create a PR for this enhancement.
For the function names, I agree that your suggested names are better. I hadn't spent much time on naming, so this feedback is helpful.
However, I have a question about the need for a separate Remove-PodeAsyncRoute. Remove-PodeRoute seems to cover the functionality since you cannot remove an async route without removing the route itself.
On the topic of merging the functionality with PodeTask, I have some reservations. PodeTask serves a very specific purpose that doesn't align perfectly with async REST calls. The latest commit includes an option to specify the maximum number of threads that each route can execute, which is a crucial feature. Some routes cannot be run concurrently or must have a limited number of concurrent executions due to the heaviness of the process.
Regarding the webhook. In this context, a callback is the appropriate outbound method. I'm going to extend the callback already in place to allow a complete interpretation of the callback semantics https://swagger.io/docs/specification/callbacks/
Another part that still I need to implement is the security. So far anyone can see everything. I need to us the roles and groups to limit access to the async results
specify the maximum number of threads that each route can execute, which is a crucial feature. Some routes cannot be run concurrently or must have a limited number of concurrent executions due to the heaviness of the process
I feel this is one that could be also achieved with Tasks as well, on top of #1037. There's need there to specify that certain tasks should only be run sequentially, and here we have a need to limit the number of a Task running concurrently, even sequentially at times - the two could probably be solved with the same solution, enabling the requirement here and enhancing Tasks at the same time:
Then 2 potential options:
-Isolated
switch on Add-PodeTask
(and Set-PodeAsyncRoute
), this enables a ParameterSet with advanced functionality to control threading - in this case likely just -MaxThreads
for now, and setting this to 1 forces sequential processing only. If MaxThreads isn't supplied then the internal default of $PodeContext.Threads.Tasks
is used.
-Isolated
create a separate Runspace Pool. Having this switch makes it safer so that people don't accidentally create a mass amount of Runspace Pools.For example:
# global
Add-PodeRoute ... | Set-PodeAsyncRoute -ResponseContentType Json
# isolated and sequential
Add-PodeRoute ... | Set-PodeAsyncRoute -ResponseContentType Json -Isolated -MaxThreads 1
or,
Add-PodeRunspacePool
public function which lets people specify a -Name
and a -MaxThreads
.
Add-PodeTask
and Set-PodeAsyncRoute
there's a new -RunspacePoolName
. If this is supplied then the Tasks run on the specified pool, if not passed then they run on the global Task pool. (It'll likely need protection to stop people running Tasks on other internal pools, hah)For example:
# global
Add-PodeRoute ... | Set-PodeAsyncRoute -ResponseContentType Json
# isolated and sequential
Add-PodeRunspacePool -Name 'CustomPool' -MaxThreads 1
Add-PodeRoute ... | Set-PodeAsyncRoute -ResponseContentType Json -RunspacePoolName 'CustomPool'
This way we don't have duplicated logic, and improve Tasks all around.
I like the idea of the isolated parameter. But to be honest, I don't see a problem with having 1000 runspaces. A runspace uses no resources other than a small quantity of memory.
In a Pode project, I'm not expecting to see 1000 async routes; if that's the case, I doubt that all of them are used simultaneously. In the end, the number of running threads is the only thing that matters
At the moment, the way it works is like this :
Add-PodeRoute ... | Set-PodeAsyncRoute -ResponseContentType Json -MaxThreads 2
As for the idea of using the same code for Task and Async, I'm only partially convinced it's feasible without compromising the compatibility with the current API.
ConvertTo-PodeEnhancedScriptBlock
does all the magic by injecting the user code inside the "async" envelope, which is completely different from how the PodeTasks are managed.
The only similar thing is Start-PodeAsyncRoutesHousekeeper,
but I want to find a way to remove it. I was thinking of using an individual scheduler to clean up each async process.
The callback implementation is completed.
Now, it is missing only the security part and the -Isolated
switch
Iβm looking at how to integrate SSE. Itβs a very useful feature when you using an async call from a browser
Documentation is done the only part missing is SSE documentation and some minor fixes to the OpenAPI definition
I'm back from holiday, so I'll begin reviewing this one and the Runspace one as soon as I can :)
Runspace is simple There are just 2 functions to make the debugging easier and a small document that explains that
Hey @mdaneri, I'm gradually getting through the review, just a slow one atm! Please try not to commit anything to the PR while I go through, as it'll confuse the ongoing review π I'm hoping to finish the rest of the review this week.
While going through work the ContentType parameters reminded me of a feature I was toying with a few months back which might actually help out a lot here. I've been mapping the idea to the work here, and so far it seems like a good match; when I get chance I'll write it up, but in short it's an alternative to the Write-PodeXResponse
functions and the way ContentTypes are figured out - and respecting the Accept header more, similar to how you have here.
I was thinking of making a small change, but I can postpone it to the next release. In this implementation, when you query for an async task, there is no limit to the number of objects you can get back. I was thinking of adding a limit of 100 configurable.
This commit introduces the Async Route feature for Pode, including Callback and Server-Sent Events (SSE) support for improved asynchronous communication.
Benefits:
Improved Responsiveness: Async Routes enable your Pode application to handle multiple requests concurrently, reducing response times and improving overall system responsiveness.
Scalability: By creating independent runspace pools, you can efficiently manage resources and scale your application to handle increased loads or complex tasks.
Enhanced Security: With Pode's security features integrated with Async Routes, you can ensure that only authorized users have access to sensitive information and operations.
Flexible Task Management: Async Routes provide a unified interface for managing asynchronous tasks, allowing you to easily create, stop, query, or callback on running tasks.
New Features:
Async.ps1
file to support the Async Route feature. These functions facilitate the creation, management, and execution of asynchronous tasks within Pode, providing developers with more flexibility and control over async operations.New Functions:
Features
Independent Runspace Pools:
Security:
Callback Support:
SSE Support:
Tests:
Async-Computing.ps1
, which evaluates the performance with 100 concurrent runspaces and over 250 parallel requests, ensuring the feature can handle high load scenarios.Example Usage:
Add-PodeAsyncGetRoute:
Add-PodeAsyncStopRoute:
Add-PodeAsyncQueryRoute:
Set-PodeAsyncRoute:
Other functions:
Get-PodeQueryAsyncRouteOperation:
Get-PodeAsyncRouteOperation:
Stop-PodeAsyncRouteOperation:
Test-PodeAsyncRouteOperation:
Are the internal functions equivalent to route operations. The only difference is that there is no security involved. The main purpose of these functions are manipulate the internal state of the async routes.