Skip to content
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

Tracking issue for std::future migration #1166

Closed
fafhrd91 opened this issue Nov 20, 2019 · 21 comments
Closed

Tracking issue for std::future migration #1166

fafhrd91 opened this issue Nov 20, 2019 · 21 comments

Comments

@fafhrd91
Copy link
Member

@fafhrd91 fafhrd91 commented Nov 20, 2019

This issue tracks the migration.

Crates migrated:

  • - actix-web
  • - actix-http
  • - actix-http-test
  • - actix-cors
  • - actix-files
  • - actix-framed
  • - actix-identity
  • - actix-multipart
  • - actix-session
  • - awc
@fafhrd91

This comment has been minimized.

Copy link
Member Author

@fafhrd91 fafhrd91 commented Nov 20, 2019

new code is in std-future branch

@fafhrd91 fafhrd91 pinned this issue Nov 20, 2019
@cdbattags

This comment has been minimized.

Copy link
Member

@cdbattags cdbattags commented Nov 20, 2019

Thanks for pushing on this so hard. Very excited to benchmark.

@fafhrd91

This comment has been minimized.

Copy link
Member Author

@fafhrd91 fafhrd91 commented Nov 20, 2019

actix-web is migrated.

anyone wants to help with other packages?

@cdbattags

This comment has been minimized.

Copy link
Member

@cdbattags cdbattags commented Nov 20, 2019

actix-web is migrated.

anyone wants to help with other packages?

I'm tracking all your commits in std-future but definitely don't have enough experience yet to get through a full migration of one of these. Happy to help on the admin side once we get closer to 2.0+ release with examples and I plan to do a full write up on the diff between 1 and 2 for personal learning and to walk through the differences.

So close!

@wigy-opensource-developer

This comment has been minimized.

Copy link

@wigy-opensource-developer wigy-opensource-developer commented Nov 21, 2019

You guys are awesome. Thanks for porting it to std::future and tokio 0.2 😄

@martell

This comment has been minimized.

Copy link
Contributor

@martell martell commented Nov 21, 2019

anyone wants to help with other packages?

@fafhrd91 You seem to be going to fast for me to help :P

I picked up actix-web-actors because it was not on the list.
Using your std-futures branch as a reference I progressed it somewhat.
I think I am about halfway through, with a few errors/mistakes currently (also lack of Pin) as it is my first time porting a futures project form 0.1 -> 0.3
https://gist.github.com/martell/1afe6fb9442e550e7b3def82c76d5c5b
I'll try to get it as close as possible tomorrow and get it into a PR for review.

@fafhrd91

This comment has been minimized.

Copy link
Member Author

@fafhrd91 fafhrd91 commented Nov 21, 2019

actix must be migrated to std::future before actix-web-actors

@martell

This comment has been minimized.

Copy link
Contributor

@martell martell commented Nov 21, 2019

Ahh I see. https://github.com/actix/actix/blob/master/Cargo.toml#L46
Thanks for the hint. I hadn't reached that wall yet, I will switch to that tomorrow evening.
I really have to ramp up my rust futures knowledge so it should be a good exercise for me.
I presume you need both actix-web-actors and thus actix ported in order to do a 2.0 release with feature parity so maybe I will be of some use at least.

@fafhrd91

This comment has been minimized.

Copy link
Member Author

@fafhrd91 fafhrd91 commented Nov 21, 2019

std-future is merged to master.

i have question, do we need sync handler? we can remove .to() and rename .to_async() to .to(), my guess most of handlers will be async

ideas?

@kiljacken

This comment has been minimized.

Copy link

@kiljacken kiljacken commented Nov 21, 2019

@fafhrd91 For migrating existing sync code to async gradually, it will be nice to keep sync support around.

@fafhrd91

This comment has been minimized.

Copy link
Member Author

@fafhrd91 fafhrd91 commented Nov 21, 2019

you don't need to change sync code, just mark sync handler with async fn

@robjtede

This comment has been minimized.

Copy link

@robjtede robjtede commented Nov 21, 2019

I agree with @kiljacken. It has to be said, however, that encouraging/easily-enabling blocking code in async contexts has been a concern during the development of futures03. .to could be tagged with a deprecated attribute with a migration explainer and set for removal in a v3 release.

@fafhrd91

This comment has been minimized.

Copy link
Member Author

@fafhrd91 fafhrd91 commented Nov 21, 2019

well, at the moment .to() is for sync code, it is not for blocking code. and difference
between sync code and async is just fn() vs async fn(), code stays the same

@robjtede

This comment has been minimized.

Copy link

@robjtede robjtede commented Nov 21, 2019

Thinking about it further, its literally 1 keyword to upgrade sync functions and anyone making the major upgrade will be reading migration docs. Async .to by default is much cleaner.

@kiljacken

This comment has been minimized.

Copy link

@kiljacken kiljacken commented Nov 21, 2019

Ahh, when you put it that way, there's probably not too much reason in keeping sync around

@fafhrd91

This comment has been minimized.

Copy link
Member Author

@fafhrd91 fafhrd91 commented Nov 21, 2019

it is done

@fafhrd91 fafhrd91 closed this Nov 21, 2019
@fafhrd91 fafhrd91 unpinned this issue Nov 21, 2019
@wigy-opensource-developer

This comment has been minimized.

Copy link

@wigy-opensource-developer wigy-opensource-developer commented Nov 21, 2019

Just a minor thing, README.md on master seems to be outdated regarding required rust version.

@kanekv

This comment has been minimized.

Copy link

@kanekv kanekv commented Nov 24, 2019

Great! I'd like to help, @fafhrd91 can you organize it so we don't step on each other toes?

@fafhrd91

This comment has been minimized.

Copy link
Member Author

@fafhrd91 fafhrd91 commented Nov 24, 2019

@kanekv you are a bit late, it is done already. Only missing piece is probably rustls and OpenSSL acceptors and connectors

@kanekv

This comment has been minimized.

Copy link

@kanekv kanekv commented Nov 24, 2019

@fafhrd91 oh, that was fast. Let me know if you need help with anything else here!

@fafhrd91

This comment has been minimized.

Copy link
Member Author

@fafhrd91 fafhrd91 commented Nov 24, 2019

there is ticket regarding tokio-openssl actix/actix-net#63
and same for tokio-rustls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.