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

Upgrade to tokio 0.3 and async-std 1.7.0 #794

Closed
wants to merge 10 commits into from

Conversation

seryl
Copy link
Contributor

@seryl seryl commented Nov 5, 2020

Upgrades dependencies to tokio 0.3 and async-std 1.7.0

I'm not completely sure how to test this locally, but cargo test passes.

@seryl
Copy link
Contributor Author

seryl commented Nov 5, 2020

There seems to be two remaining issues:

  1. TlsConnector seems like it's not being used for async-std requests (may be unrelated to our changes).
error occurred while attempting to establish a TLS connection: operation would block (unable to get local issuer certificate)
  1. Actix isn't starting the tokio engine first; I'm not quite sure why this is occurring.

@abonander
Copy link
Collaborator

Actix has not yet updated to Tokio 0.3 so actix-rt starts a Tokio 0.2 runtime, not 0.3. Until they upgrade, I'm not sure that we can merge this because actix-web is a core part of our development stack and we need to be able to use SQLx in request handlers.

@nkconnor
Copy link
Contributor

nkconnor commented Nov 9, 2020

FYI, the PR: actix/actix-net#204

@ant32
Copy link
Contributor

ant32 commented Nov 13, 2020

I made a pull request seryl#1 with master merged and actix/actix-net#204 mentioned by @nkconnor

And tests all pass https://github.com/ant32/sqlx/runs/1394187958

@abonander
Copy link
Collaborator

@seryl if you're still interested in finishing this PR, Tokio 1.0 is out now with minor breakages from 0.3. Do you want to rebase and update?

@mehcode
Copy link
Member

mehcode commented Dec 27, 2020

If we pull apart the actix and tokio implementations in sqlx-rt, we should be able to merge this and leave actix pulling in tokio 0.2.

We'll probably need to add:

tokio_02 = { version = "0.2.24", optional = true, package = "tokio", features = ["net"] }.

or similar and include that when runtime-actix-* is picked

@seryl
Copy link
Contributor Author

seryl commented Dec 28, 2020

I am still interested in working on this. Let me merge these into a single request with the 1.0.1 update; I have a feeling there might be some additional things with have to handle with the given updates.

@mehcode is there anything additional you'd like me to do outside of bumping to 1.0.1 and getting it into a buildable position?

Also; which tls are you folks using for the Cargo.lock?

@mehcode
Copy link
Member

mehcode commented Dec 30, 2020

@mehcode is there anything additional you'd like me to do outside of bumping to 1.0.1 and getting it into a buildable position?

Nope. I think you just need to split up the imports so we import from actix_rt or tokio_02 and not tokio for runtime-actix-* and we can merge this.

@vicky5124
Copy link

Actix merged the tokio 1.0 switch actix/actix-web#1813
This pr should change to update tokio to 1.0 and should no longer be blocked.

@ant32
Copy link
Contributor

ant32 commented Jan 5, 2021

I could make a pull request but the work is largely based on @seryl's work.
squashed: master...ant32:tokio-1.0
Tests: https://github.com/ant32/sqlx/actions/runs/464410714

@jplatte
Copy link
Contributor

jplatte commented Jan 5, 2021

This still needs to wait for actix-rt 2.0.0 to be released AFAICT.

@ant32
Copy link
Contributor

ant32 commented Jan 5, 2021

@jplatte I did not expect it to be ready to be merged. It needs some cleanup. I wanted to use this for a personal hubby project where I am already using tokio 1.0 and actix-web master. For merging now I would have imagined we would need to follow #794 (comment) idea and split some stuff up. But it might not be worth splitting it up if actix-rt 2.0 would get released soon? Someone else can gladly take my work and finish it if it is useful.

@mehcode mehcode mentioned this pull request Jan 8, 2021
@vicky5124
Copy link

The beta versions for the various actix libraries have been released. This PR is no longer blocked by the release of those, and a tokio 1.0 beta release should be able to be done with them.

@adobeDan
Copy link

@seryl I need a tokio 1.0.1 port of sqlx for an open source project I am working on. I see from your comment of a couple of weeks ago that you are still working on this. If it would be helpful, I am happy to try to complete it for you. Just let me know.

@Gelbpunkt
Copy link

@ant32's branch with tokio 1.0 and actix-rt 2.0 works fine for me, maybe one could merge from that source instead?

@adobeDan
Copy link

Yes and @ant32's branch (no PR yet) seems to maintain support for rustls which is not in this pull. @ant32 is yours ready to merge or do you still want to do cleanup?

@ant32
Copy link
Contributor

ant32 commented Jan 11, 2021

I think it is fine to merge but not release as a stable release since the actix dependencies are still in beta. I would also have expected the owners of this repository to have a cleaner way to handle some of the breaking changes that I created workarounds for. I don't think there is anything blocking you from just using my git repository for the time being and there no rush to merge until actix is stable.

[dependencies]
tokio = { version = "1", features = ["full"] }
sqlx = { git = "https://github.com/ant32/sqlx.git", branch = "tokio-1.0", features=["runtime-tokio-rustls"] }

@adobeDan
Copy link

@ant32 Thanks for the summary of status. I will start testing with your repo in our project; if I find any issues I'll try to fix them and let you know.

@adobeDan
Copy link

@ant32 Thanks so much! Your branch worked perfectly in our project which uses the latest tokio and native-tls.

@mehcode As a fan of this project, I think it would be great to ask @ant32 to do a push so you can make a topic branch or beta release of it. That way at least other folks on the latest tokio will find a working sqlx in this repo.

@mehcode
Copy link
Member

mehcode commented Jan 14, 2021

I didn't realize actix-rt is still in beta. We do need to wait for a stable release of that.


But to your point @adobeDan, we could do a -beta release ourselves.. hm.


I think it is fine to merge but not release as a stable release since the actix dependencies are still in beta. I would also have expected the owners of this repository to have a cleaner way to handle some of the breaking changes that I created workarounds for. I don't think there is anything blocking you from just using my git repository for the time being and there no rush to merge until actix is stable.

@ant32

Do you mind opening a PR for that branch? We can merge and push a -beta release. I don't care too much about whatever hack you needed to do in the runtime support. That's all being removed in 0.6 (and replaced with type generics for runtimes).

@jplatte
Copy link
Contributor

jplatte commented Jan 14, 2021

I've had a look at ant32's changes, there are some things that seem undesirable. I'll try to clean it up and post a new PR.

@mehcode
Copy link
Member

mehcode commented Jan 21, 2021

Superseded by #983

Thanks all

@mehcode mehcode closed this Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port sqlx to latest tokio, async_std, rustls, and native_tls
9 participants