-
Notifications
You must be signed in to change notification settings - Fork 342
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
actix-rt: Make the process of running System in existing Runtime more clear #173
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8f8bee2
actix-rt: Make the process of running System in existing Runtime more…
popzxc 9cb8760
Update changelog
popzxc 9a19b08
Review fixes
popzxc 8e7f950
Fix clippy warning inherited from master
popzxc 11b2d2b
Merge branch 'master' into friendly-tokio
robjtede File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 justjoin
ing 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I've attempted several other approaches, but found them not as convenient because of the following factors:
LocalSet
occupies current thread forSystem
to run;LocalSet
is 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 anasync
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 anasync { }
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like we're moving into a simpler Node.js style
<req, res>
or Koa'sctx
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is similar to what @fafhrd91 was describing too about the
actix-net
features.