actix-rt: Make the process of running System in existing Runtime more clear #173
Conversation
Codecov Report
@@ Coverage Diff @@
## master #173 +/- ##
==========================================
- Coverage 60.20% 60.13% -0.07%
==========================================
Files 73 73
Lines 4784 4789 +5
==========================================
Hits 2880 2880
- Misses 1904 1909 +5
Continue to review full report at Codecov.
|
| /// let rest_operations = run_application(); | ||
| /// System::attach_to_tokio(runtime, "actix-main-system", rest_operations); | ||
| /// ``` | ||
| pub fn attach_to_tokio<Fut, R>( |
There was a problem hiding this comment.
I like the idea of this function, making it much simpler to host actix in an existing tokio runtime.
Are there alternate designs for the rest_operations idea worth discussing? I seem to recall in the past just joining and awaiting the actix LocalSet with any other long running tasks. This approach feels more opinionated and less extensible.
There was a problem hiding this comment.
Well, I've attempted several other approaches, but found them not as convenient because of the following factors:
LocalSetoccupies current thread forSystemto run;- Future, obtained via
LocalSetis notSend.
Thus, returning a future from this function (instead of blocking the runtime inside) makes the user-side code not really intuitive.
As an alternative approach, we may make rest_operations a function rather than a future, but IMHO it's less flexible: if you have an async function, you can get a future by simply calling it, and at the same time you can get a future without an additional function (e.g. using an async { } block).
Or do you have an idea how to get rid of the rest_operations argument without complications on the caller site?
There was a problem hiding this comment.
This feels like we're moving into a simpler Node.js style <req, res> or Koa's ctx and I'm all for it. I'd love to have Actix just be the web layer on top of default Tokio.
There was a problem hiding this comment.
I think this is similar to what @fafhrd91 was describing too about the actix-net features.
|
@robjtede Can you please clarify how long it usually takes for a PR to get reviewed? |
|
@popzxc I'm in favor of these changes. I'd like someone else from @actix/contributors team to chime in before we get it merged though. |
|
I see, thanks! Let's wait then :) |
|
@robjtede I don't want to be annoying, but it seems that nobody else want to review this PR. |
|
@popzxc I just looked over the changes. I think it would be good to add a test for this new function. Also the PR will need to be rebased against master. Other then that it looks good. Ping me later if you need another review. |
@Neopallium Isn't provided doc-test enough? If not, I'm not exactly sure what kind of test is required, could you provide an approximate form of how do you see it? |
|
I am not sure if doc-tests are run. checking now. |
|
Also after rebase there seems to be some inherited clippy warnings. Not sure if I have to fix them as part of this PR, as some of them don't even belong to the |
|
yeah. I see those. I don't think they should be fixed in this PR. I can't merge this PR, but I can submit a review for you. The Doc-tests seem to only check if the doc example code compiles. A test would be good to make sure that attaching to an existing tokio-runtime still works (all normal tokio & actix tasks still run). I am not sure if that would be easy to verify in a test. |
@Neopallium nope, many thanks for your input here. Once the rebased checks pass we'll merge. |
PR Type
Other (Usability enhancement)
PR Checklist
Check your PR fulfills the following:
Overview
When I first tried to start the
actix-webserver using an existingtokioRuntime, I honestly found the interface a bit unfriendly.In an attempt to make other folks' life easier I did the following:
Arbiters still using their ownRuntimeobjects.Systemto a givenRuntime.These changes aren't breaking and hopefully useful.