actix-web icon indicating copy to clipboard operation
actix-web copied to clipboard

chore: update http crate

Open morenol opened this issue 1 year ago • 8 comments

Depends on https://github.com/actix/actix-net/pull/508

PR Type

Updates http crate

PR Checklist

  • [ ] Tests for the changes have been added / updated.
  • [x] Documentation comments have been added / updated.
  • [x] A changelog entry has been made for the appropriate packages.
  • [x] Format code with the latest stable rustfmt.
  • [ ] (Team) Label with affected crates and semver status.

Overview

morenol avatar Dec 02 '23 16:12 morenol

I'm curious if you have any particular motive to get this merged. From my PoV, the Actix Web project doesn't have any real need to do it.

robjtede avatar Dec 02 '23 17:12 robjtede

Thanks for your feedback! I don't have any particular motive other than having that dependency up to date, so other crates that have actix-* as dependency can also update their http dependency to the latest one

morenol avatar Dec 02 '23 17:12 morenol

Not really sure how that happened but:

# cargo tree -i [email protected]
http v1.0.0
├── actix-tls v3.3.0
│   ├── actix-http v3.6.0
│   │   ├── actix-web v4.5.1
│   │   │   ├── actix-cors v0.7.0
...

whereas the rest of the actix universe is depending on 0.2. It is a bit unfortunate that one set of actix crates pulls in one dependency in two different versions.

therealprof avatar Feb 07 '24 10:02 therealprof

Context for that particular change: https://github.com/actix/actix-net/pull/508

robjtede avatar Feb 07 '24 11:02 robjtede

I think switching to and using a 1.0 version is always a good venture due to the implied stability guarantees.

therealprof avatar Feb 07 '24 12:02 therealprof

Is there anything blocking this PR?

juchiast avatar Mar 22 '24 03:03 juchiast

I'm curious if you have any particular motive to get this merged. From my PoV, the Actix Web project doesn't have any real need to do it.

One use case is passing header values extracted from actix's request to other libraries, such as reqwest 0.12 . Because of incompatible http version, we have to convert HeaderValue to bytes before passing.

juchiast avatar Mar 25 '24 08:03 juchiast

I'm curious if you have any particular motive to get this merged. From my PoV, the Actix Web project doesn't have any real need to do it.

Inevitably security support for http 0.2.x will cease long before security support for http 1.x does.

linuxtim avatar Apr 02 '24 18:04 linuxtim

If I understand correctly from the milestone, the maintainers are waiting for the v5.0 release to merge this? Hopefully this is done sooner than later, as there is 0 reason for any actively maintained library to still be using http version 0.2.x.

SleeplessOne1917 avatar May 30 '24 13:05 SleeplessOne1917

There's no security risk of staying on v0.2. It's my opinion that it's not worth a breaking change just for this. The ecosystem churn of releasing Actix Web v5.0 is vast; far outweighing any potential benefits of sticking with this older version until enough good reasons exist to break everyone.

robjtede avatar May 30 '24 13:05 robjtede

I can see holding this off until the next major version bump since it is, as you said, a breaking change. The thing that drew me to this issue is the one @juchiast mentioned: I'm unable to upgrade the reqwest dependency in one of my projects to 0.12.x without doing a bunch of workarounds. As the documentation for the crate says says:

It’s intended that this crate is the “standard library” for HTTP clients and servers without dictating any particular implementation.

With the crate's status as standard to be used by client and server crates, and with many of those crates updating to use the 1.x version, the incompatibility issues will only increase as time goes on.

SleeplessOne1917 avatar May 30 '24 14:05 SleeplessOne1917

Sorry, but compatibility with the hyper ecosystem isn't one of my concerns.

I disagree that http, despite its name, should be considered as highly as "the standard library" for HTTP in Rust. Also FWIW, while this upgrade is one option for the v5 release, it's also possible I'll move Actix Web off that crate entirely.

robjtede avatar May 30 '24 14:05 robjtede

fwiw i already did the reqwest 0.12 upgrade in pict-rs and the only thing I needed to add was a conversion between http1 and http02 status codes, which I did with this code:

pub(crate) fn to_actix_status(status: reqwest::StatusCode) -> actix_web::http::StatusCode {
    actix_web::http::StatusCode::from_u16(status.as_u16()).expect("status codes are always valid")
}

asonix avatar May 30 '24 14:05 asonix

I'm a little late to discussion.

I'm curious if you have any particular motive to get this merged. From my PoV, the Actix Web project doesn't have any real need to do it.

From a p.o.v. of Actix (maintainer) indeed there might not be much motivation. From an Actix user however there are. Currently we have two versions of the same crate in our dependency tree due to Actix, the leads to (among other this) larder binary and increased build time. This also leads to slight incompatibility between since as highlighted by @asonix (https://github.com/actix/actix-web/pull/3208#issuecomment-2139818375).

There's no security risk of staying on v0.2. It's my opinion that it's not worth a breaking change just for this. The ecosystem churn of releasing Actix Web v5.0 is vast; far outweighing any potential benefits of sticking with this older version until enough good reasons exist to break everyone.

This is totally a fair point of view. And thank you for not creating churn for no reason (it's one of my pet peeves).

Also a note on how this pr is implemented; it won't work. If a single dependency decides to enable the http-1 feature it will enable it for all, meaning that it will create a mess of incompatible crates and crate versions. If the switch to http v1 is made it should not be optional or dependent on a feature flag.

Thomasdezeeuw avatar May 31 '24 16:05 Thomasdezeeuw