#tower-internals

406 messages · Page 1 of 1 (latest)

tender oar
tender oar
dry spruce

Mentioned on the 0.5 release issue

sly zephyr

I've made a decent sized experiment with a different Service trait.

  • Includes LoadShed, Oneshot, ConcurrencyLimit, ReadyCache, Then, Map, Buffer etc
  • First class disarming via permits (Service contract encoded in the API)
  • &self rather than &mut self
  • Works on stable via GATs, with simple migration to async fn in traits.

I'd like to share the approach, where is the best place to do so?
Here https://github.com/tower-rs/tower/issues/753 ?
A new issue?
Just link it here?

tender oar
zinc rune

If it's different enough, you could also open a new issue and just be sure they are linked

dense anchor

is there a simple example / tut to how to create a middleware tower from a func ?

tender oar

That’s a question got for #tower ? Unless you ask because you want to contribute?

With middleware you mean layer I assume? And that’s really just a Service which wraps around service.

A layer produces a service from another service. So you can combine layer_fn (https://docs.rs/tower/latest/tower/layer/fn.layer_fn.html) by having it produce a service using service_fn (https://docs.rs/tower/latest/tower/fn.service_fn.html).

So that would basically be a closure returning another closure. A regular fn won’t work with these as those can’t have move state in them which you need in that setup. Personally I prefer to have explicit types for my services though. Easier to organise and reason imho.

That said… you can also create a ‘middleware’ service using a (static) fn (so regular function), by using https://docs.rs/tower/latest/tower/layer/util/struct.Stack.html. That would allow you to create the middleware (layer) using layer_fn to return a stack of your service_fn service as inner and the moved received service as outer. Does give up some freedom of control, but it’s a legit option. In fact… ServiceBuilder methods often use Stack themselves.

zenith pulsar

for the next major version of axum we're considering to require all services used in the router be Sync. In the past we've only required Send, not Sync.

With Sync we'll be able to make Router an Arc internally so its cheaper to clone. The router is currently cloned quite a bit while handling requests.

Relevant PRs:
https://github.com/tokio-rs/axum/pull/2473
https://github.com/tokio-rs/axum/pull/2476

For people routing to handler functions I don't think that'll be a problem but I'm not sure if anyone out there are writing !Sync services 🤔

Anyone here who have ever worked with a !Sync service?

tender oar

Interesting for sure @zenith pulsar .
For Rama I am now in my 5th iteration...

Having switched between tower, and tower-async and back to tower. I am now at not using Tower at all, but having something based on all learning from previous iterations...

For now I am requiring my services only to be Send, with the signature also having &self.

But that does mean that I often need to Clone for layers where I need to use the inner services across an await.

Same for using my Service as a Hyper Service, it also means I need to Clone for each (http) requrest, as I can otherwise not implement the that trait due to having to Box my future...

Once I can use impl Trait for associated types that no longer would be an issue...

A shame though as everything else is working fine for me without having to Box futures. I can even use dynamic dispatch on stable rust (1.75) for my async fn traits now, so pretty happy so far.

But yeah... hmmm... interesting... Was considering to require my services to be Sync as well, but didn't think it would be a reasonable thing to do.

For Axum I get that it's ok as the Router handles are only Functions, no? But for middleware that would be more tricky as there you might want to have state inside of the middleware services which would make requiring Sync a bit of a no-go, I think?

tender oar

Hmmmm… you made me think… requiring sync for my services and state might be actually reasonable … something to think about

Going to try that out today during one of Nap times of the smallest one

I think my original motivation was because one of the last async rust blogs was saying that most traits would only ever have to be Send. But I think in our situation it’s reasonable to also demand Sync.

vernal spade

Multi-threaded, need to share routing objects, if there is no Sync, we can not use Arc, it will cause memory growth when cloning.

tender oar
silk grotto

Hi all, I'm new to the community, I raised a feature request for tower-http and wanted to have a pair of eyes look at implementation proposal before I raise my PR.
Is this the right channel for tower-http questions?
https://github.com/tower-rs/tower-http/issues/466

dry spruce

Very well written issue, thanks!

And yeah, here is a good place to discuss things like that

silk grotto
dry spruce

No need to ping here, I get notified on GitHub

random badge

I before found someone said tower design follows another software from twitter？ which software？

random badge
tender oar

Was going over tower-http changes since the 0.5.0 release, as I had gotten a bit behind on following that progress.

I noticed https://github.com/tower-rs/tower-http/tree/main/tower-http/src/cors <- there was a tests.rs mod added, but it doesn't seem to be enabled in mod.rs,

e.g. using

#[cfg(test)]
mod tests;

Is that on purpose, or shall I add a PR for it?

tender oar

cors/tests.rs not active?

sly zephyr

Unbaked idea: Is there a world where tower yeets all(?) MiddlewareLayer impls and instead

impl<S, T> Layer<S> for Middleware<T>
where
    T: Layer<S>
{
    type Service = Middleware<T::Service>;

    fn apply(self, svc: S) -> Self::Service {
        // Swap self.inner for self.inner.apply(svc)
    }
}

and there's a distinguished

struct Placeholder;

impl Service<!> for Placeholder { ... }

impl<S> Layer<S> for Placeholder {
     type Service = S;

     fn apply(self, svc: S) -> Self::Service {
          svc
     }
}

Then building a Layer would be the same as building a Service around Placeholder.

sly zephyr
quick flame

I found this BoxCloneSerivce that implements Sync in axum. Would this be sth that could be added to tower itself like the original BoxCloneService?

sterile notch

I'm also interested in this question. Right now I also have a vendored version of BoxCloneService with an additional Sync bound in my project.
If that's something multiple projects have, it might be worth adding it to tower itself

tender oar

Would be a breaking change that requires a major version. It's also worth nothing that the Sync bound is not needed for all environments for which tokio/tower can be used. And of course tower is not even bound directly to tokio itself. But even within tokio you do not need it in all setups.

Don't get me wrong, for my own projects (tower-async, rama) I also use the infamous bound triplet (send + sync + 'static ) all over the place, but that is because I just force that upon myself. Not sure tower ever wants or needs that.

Then again, don't think it matters much what I think 😛 So better wait for someone with more authority to weight in 😓 😄

quick flame
daring wigeon
hoary cargo

@daring wigeon does that actually work now? and doesn't force Send / !Send? Also how does that fit w/ async fn?

i'm guessing you would have to box the async { } when implementing the trait

daring wigeon

async fn is still waiting for a new edition to set up not forcing send or !send.

I’m not sure about whether this forces Send/!Send, come to think of it. I need to experiment more

hoary cargo

@daring wigeon I could be ok w/ ^ for a tower 0.5 before 2024 if you want to take it on, there are some other changes worth experimenting with as well

e..g imo poll_ready should be removed (in favor of a separate trait) and Error removed in favor of having the response be Result if needed

daring wigeon
hoary cargo

getting rid of Error would fit better w/ something like axum or other http based services where error is already baked into http::Response

we could probably pair the trait w/ GAT w/ a proc macro like async-trait for implementation

@daring wigeon oh call should probably take &self

daring wigeon

Hmm, why? I figure &mut self gives a lot more versatility, and you can always implement Service on &T

hoary cargo

mostly because most state will be handled in the future

and actual usage of the service (tonic, axum, ...) almst always work w/ Arc<Service>

also, it can always be reversed in 1.0

daring wigeon

Right; I’m just trying to think of cases that would be broken by this

hoary cargo

i don't really think many will and there is always interior mutability

patterns that do need a mut step can decouple the service from getting a "ready" service

get_my_service().await.call(...)

worth an experiment though, and if thijngs break and there are no good new patterns, we can evaluate reversing it

all this is based on my recollection of informal discord chats a long time ago

maybe @iron delta @zenith pulsar @zinc rune have other insights there

tower's biggest problem right now imo is it is really painful to actually use

so simplifying it is important

@daring wigeon another thing to keep in mind, tower's current trait is from the futures 0.1 days...

so ithink reevaluating all assumptions and target async fn is a good idea

daring wigeon

Right, makes sense

Yeah, I figure that poll_ready can be replaced with a Service that yields another Service...

hoary cargo

also, GAT changes everything as well

daring wigeon

Yeah now we don't need to lug lifetimes around everywhere

dry spruce

Re. simplification, I think an artificial Send requirement that allows assuming the bound in generic code is probably the best we can do now, then the next semver-incompatible version after would reverse that, blocking on return type notation or type alias impl trait (in traits), which would then allow generic code to impose the Send bound without it being required by the trait def.

tender oar

I have mostly user experience, and also practical experience with the tower-async experiments that ended up with a more permanent fork directly in rama. So if I can be of any feedback or use, do let me know 🙂 Happy to chip in if you think it can be useful.

dry spruce

@tender oar didn't you also previously offer to help w/ tower maintenance? Or am I misremembering?

tender oar

Yes. That offer is still there as it has given me already so much.

dry spruce

I'm not a maintainer myself so I can't do much but it's nice that I'm not misremembering 😄

sly zephyr
shadow crater

straw poll: any good reason why we shouldn't start blanket approving basic PRs on the tower repo? basic PRs being.... improve docs, update dependencies to their latest major versions, etc

seems like it might be worth it to consider a big PR backlog push and a move to 0.5, while ignoring anything deeper (like splitting poll_ready out of Service, etc)

tender oar

Fully agreed and something I don’t even mind helping with it now and as a maintainer in general.

mortal cradle

"Blanket approving PRs" gives me a "Jia Tan" kind of worry.

But, then again, I'm not a Tower dev, so...

tender oar

Unfortunate choice of words. I interpret it as get the PRs moved forward that are obviously fine but have been open for long only because no current maintainer has time for it.

Which is fair. Open source is often voluntary work and we should therefore be grateful it is even there. If however other people can put time in it to keep it going forward then that at least should be considered. Not an obligation obviously to accept new maintainers, there’s a lot of trust in there.

shadow crater

yeah, I'm saying "blanket approval" in the sense of not bothering to get multiple reviews before merging, etc

I think a lot of what sometimes gets in the way is people feeling like there needs to be a lot of consensus because nobody is really a designated maintainer of Tower these days

no BDFL, as it were

grim laurel

Yeah, would be nice to just get some moving through, even if no huge changes

shadow crater

@zinc rune @waxen stag @iron delta curious for your thoughts here if you have some time, as I think y'all are the most familiar with the long-term plans for tower, whether official or unofficial

zinc rune

happy to work up some sort of regular triage system

i think several repos could use one, frankly

as for pushing out a breaking change, i dunno... if it's to some of the middleware in tower or tower-http, that should be more fine (does axum or tonic openly depend on them? RIP if so)

shadow crater

yeah, my thought process is that any breaking changes that aren't a consequence of changing dependencies (so like the indexmap is an example of this) would not get caught up in the PR merge bonanza that I'm suggesting

zinc rune

if its just an internal dep change (and msrv is fine), who cares

well, within reason. its a fundamental crate, so we should vet any dependencies we add...

shadow crater

yeah, there's definitely some of those, but indexmap is an example where it involves a breaking change to the API of... one of the middlewares that use it (IIRC... I'd have to go refresh my memory since there's a few open PRs trying to do the update for indexmap)

zinc rune

hm, its taking away a method? will someone likely eventually need that method added back, and we need to juggle the dependency to allow it?

if we did re-add it, it seems unfortunate to go through the breakage...

shadow crater

yeah, I need to dig into it more but it sounds like you dont have any strong opposition to the overall sentiment?

shadow crater

fwiw, I'm going ahead and starting to approve/merge simple stuff in the PR backlog

tender oar

Thx a lot for the momentum you’re bringing forward for tower @shadow crater and your hard work that comes with it 🫡💪

If there’s something I can do to help, do let me know!

shadow crater
tender oar

You can not just merge it via cli? Or is master/main protected?

rich panther

oh, I guess I don't have permissions there

I didn't know we had a different tower org

shadow crater
tender oar

@shadow crater i can prepare a PR for https://github.com/tower-rs/tower/issues/768 if you think they could still go in the current milestone.

You seem to hint on some extra work like the wrappers and what not. This hints to me at certain design decisions you have in mind. If that assumption is correct it would help if you can also write them down so I can already take it into account for first draft.

I recently also switched to futures-lite in the plabayo/rama project.

shadow crater

no particular design in mind; just that thinking through what it might mean to eventually release tower 1.0, it implies deps that are also 1.0

futures, and all futures-* derivatives, don't fit that

so hiding things behind types owned by tower is the easiest solution (this is what tokio and hyper did, to an extent, to handle that)

tender oar

Ok gotcha. It can still go in current release? So i know to put some priority behind this or if that’s not useful effort

shadow crater

yeah, I don't see why not

i'm not planning on trying to cut a release tomorrow or anything, just trying to get as much merged in and organized as is reasonable, and maybe think about a release in 1-2 weeks

tender oar

Ok if there’s other stuff you can off load do post it here 🙂

shadow crater

yeah, I mean, in general: going through and tagging issues is helpful, finding duplicates and closing them, etc

actually, I guess I don't know if you can label things if you're not a maintainer

tender oar

I cannot. Plus don’t want to start walking on your toes or doing duplicate work.

Don’t need maintainer rights for tagging. i think most basic rights as member would allow that already however

tender oar
shadow crater

it seems reasonable to me at face value, but I don't know the usecase, and have never personally hit a problem where this was needed, so it's hard to grok if this would introduce subtle issues or something

rich panther

People will need to add 'static to their types, which could be confusing. Otherwise I don't think it's problematic.

shadow crater

hmm, yeah, that's a good point

tender oar

Feels like more of a 1.0 thing than 0.5, but might be just me.

frozen pier

does anyone have objections to to doing a 0.5 release of tower? i talked to @iron delta over DMs about some related stuff, but i'd very much like to use the unreleased retry stuff

(i can also copy-and-paste it in, but for a decently foundational library, that feels bad...)

iron delta

I have no objection

frozen pier

do you want to a 0.5-alpha.1 release?

iron delta

that could be a good idea if there are some things we are not sure about

but imo it just crates another breaking release

so if you feel confident I would say just go for it

frozen pier

there are already some breaking changes; i wasn't sure if 0.5 was intended to be reserved for an RTN'd async trait

okay, i'll prepare a release then

frozen pier
iron delta

I think tower itself is better to just bump without the alpha

I think we should drop the alpha and do 0.5 of tower imo

frozen pier
iron delta

So can just make that 0.6

frozen pier

alright, sure—if updating tower is cheap, then i can make it 0.5

i don't believe i can release: would you be willing to do the honors?

done, pushed

shadow crater

seems like the retry stuff could also just land in patch releases

ain't a breaking change... is it? victhinking

frozen pier

no, it is a breaking change

it's a signature change

shadow crater

related to 0.5: are we just waiting on someone who can publish it to crates.io?

or was there any specific testing people wanted to do still?

frozen pier

yes, i think it's just waiting on someone to publish it to crates.io

shadow crater

word; looks like i don't have permissions

i'm guessing it's eliza/lucio/carl/sean since they're owners on the tower-rs org

11am minus 3 hours... maybe i'll wait a little while before mass pinging them kekw

frozen pier

i think we can bother sean and lucio, as they're are both in new york!

shadow crater

TIL about sean being in NY

@iron delta @zinc rune would either of y'all want to publish the latest versions of tower-service, tower-layer, and tower?

iron delta
shadow crater

yeah

frozen pier

yeah, what toby said

iron delta

@shadow crater @frozen pier is it already tagged n stuff?

shadow crater

not tagged; I'll do that right now

tagged:

Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
To github.com:tower-rs/tower.git
 * [new tag]         tower-0.5.0 -> tower-0.5.0
 * [new tag]         tower-layer-0.3.3 -> tower-layer-0.3.3
 * [new tag]         tower-service-0.3.3 -> tower-service-0.3.3
iron delta

uhhhh

❯ cat Cargo.toml
[package]
name = "tower-layer"
# When releasing to crates.io:
# - Update doc url
#   - Cargo.toml
#   - README.md
# - Update CHANGELOG.md.
# - Create "v0.1.x" git tag.
version = "0.3.3"
authors = ["Tower Maintainers <team@tower-rs.com>"]
license = "MIT"
readme = "README.md"
repository = "https://github.com/tower-rs/tower"
homepage = "https://github.com/tower-rs/tower"
documentation = "https://docs.rs/tower-layer/0.3.3"
description = """
Decorates a `Service` to allow easy composition between `Service`s.
"""
categories = ["asynchronous", "network-programming"]
edition = "2018"

[dependencies]

[dev-dependencies]
tower-service = { version = "0.3.0", path = "../tower-service" }
tower = { version = "0.5.0", path = "../tower" }
tower/tower-layer on  master is 📦 v0.3.3 via 🦀 v1.80.0
❯ cargo publish
    Updating crates.io index
   Packaging tower-layer v0.3.3 (/home/lucio/code/tower/tower-layer)
   Verifying tower-layer v0.3.3 (/home/lucio/code/tower/tower-layer)
    Updating crates.io index
error: failed to verify package tarball

Caused by:
  failed to select a version for the requirement `tower = "^0.5.0"`
  candidate versions found which didn't match: 0.4.13, 0.4.12, 0.4.11, ...
  location searched: crates.io index
  required by package `tower-layer v0.3.3 (/home/lucio/code/tower/target/package/tower-layer-0.3.3)`
  if you are looking for the prerelease package it needs to be specified explicitly
      tower = { version = "0.3.0-alpha.1a" }```

why does it require tower as a dev dep to publish

should I just comment out that part of the toml?

btw service is out

shadow crater

that's weird, yeah; let me see what the previous versions do

frozen pier
iron delta

is this a change in cargo?

shadow crater

oh, i see: you need to release tower first, then tower-layer

tower-service doesn't depend on tower, which is why the publish worked

dry spruce

Cyclic dev-dependencies are the best! /s

iron delta

I thought tower depended on layer lol

no?

shadow crater

only as a dev dep

dry spruce

No the dev dep should be the other way around

iron delta
shadow crater

errr, sorry: you're right (I was reading your statement backwards)

iron delta

lol

this is 100% a new compiler error

dry spruce

But regular dep minimum version is low enough for publish of tower 0.5.0 to work 🙂

iron delta

im on 1.80

dry spruce

(before 0.3.3 of -layer)

iron delta

true

good point

doesn't feel good tho 🤣

well well well

tower/tower on  master is 📦 v0.5.0 via 🦀 v1.80.0
❯ cargo publish
    Updating crates.io index
   Packaging tower v0.5.0 (/home/lucio/code/tower/tower)
    Updating crates.io index
   Verifying tower v0.5.0 (/home/lucio/code/tower/tower)
  Downloaded tower-service v0.3.3
  Downloaded 1 crate (7.0 KB) in 0.07s
   Compiling tower-service v0.3.3
   Compiling tower-layer v0.3.2
   Compiling tower v0.5.0 (/home/lucio/code/tower/target/package/tower-0.5.0)
error[E0015]: cannot call non-const fn `Identity::new` in constant functions
   --> src/builder/mod.rs:120:20
    |
120 |             layer: Identity::new(),
    |                    ^^^^^^^^^^^^^^^
    |
    = note: calls in constant functions are limited to constant functions, tuple structs and tuple variants

For more information about this error, try `rustc --explain E0015`.
error: could not compile `tower` (lib) due to 1 previous error
error: failed to verify package tarball
dry spruce

You found a bug! Wrong minimum version 😄

I love that cargo publish tests against what's published before actually publishing, by default.

shadow crater

jfc

dry spruce

So yeaaaah I'm pretty sure the canonical solution here is to comment out the dev-dep 🫠

iron delta

and ill do the dirty work for layer

dry spruce

(maybe after fixing the min ver bug, though now the releases are already all tagged /o\)

iron delta

just bump the tag to 0.5.1

and 0.5.0 just never gets released

shadow crater

well, we can rewrite the tags for everything but tower-service

they aren't published yet

we're not abusing anything by doing so

dry spruce
iron delta

so should I publish layer rn or not?

dry spruce

Rewriting the tag is also not the worst possible solution

shadow crater

i think i'm missing the understanding on the "minimum versions" thing on why it fails to publish

iron delta

ah I know the fix

remove the version from the dev dep

dry spruce

But yeah, I think first step is gonna be releasing tower-layer as-is, anyways.

iron delta 
    Updating crates.io index
   Packaging tower-layer v0.3.3 (/home/lucio/code/tower/tower-layer)
   Verifying tower-layer v0.3.3 (/home/lucio/code/tower/tower-layer)
    Updating crates.io index
   Compiling tower-layer v0.3.3 (/home/lucio/code/tower/target/package/tower-layer-0.3.3)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.38s
    Packaged 10 files, 23.5KiB (5.9KiB compressed)
   Uploading tower-layer v0.3.3 (/home/lucio/code/tower/tower-layer)
warning: aborting upload due to dry run
tower/tower-layer on  master [!] is 📦 v0.3.3 via 🦀 v1.80.0
❯ gd
diff --git a/tower-layer/Cargo.toml b/tower-layer/Cargo.toml
index fa4beee..1f3fa49 100644
--- a/tower-layer/Cargo.toml
+++ b/tower-layer/Cargo.toml
@@ -23,4 +23,4 @@ edition = "2018"

 [dev-dependencies]
 tower-service = { version = "0.3.0", path = "../tower-service" }
-tower = { version = "0.5.0", path = "../tower" }
+tower = { path = "../tower" }
shadow crater

removing the version makes sense to me, actually, yeah

iron delta

still DO NOT get why it verifies the version of dev deps makes no sense

on publish that is

dry spruce

You'd be surprised

We got at least one bug report in axum for the crate tarball from crates.io failing cargo test

iron delta

huh who tf is running cargo test on that

I guess crater?

dry spruce

Nono, not crater

I think it's Debian actually

iron delta

ofc debian 😂

well this prob wouldn't be a problem for them for at least another 4 years they are probably still on 1.45

dry spruce
shadow crater

@iron delta do you wanna push a commit/PR for fixing the dev dep or should i?

iron delta
shadow crater

yep, standby

iron delta

just ping me here when its ready

shadow crater

i can merge and re-tag quickly once that's approved

iron delta

yeah merge and tag and then ill publish

I appreciate it

shadow crater

@iron delta all set

iron delta

BTW

warning: trait `Sealed` is never used
   --> src/lib.rs:223:15
    |
223 |     pub trait Sealed<T> {}
    |               ^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: `tower` (lib) generated 1 warning

they are all published

but there is this warning

when I published

shadow crater

huh, weird... wonder if that's been showing up in the CI, too (curious if it's new or old)

tender oar

Given the complaints are coming in it might be good to start a similar effort on tower-http, etc? As there’s no good reason other than dev time to have this be not an issue?

Let me know if I can play a part in helping making this happen

dry spruce

@tender oar what complaints are you talking about? I'm happy to do review & merge on tower-http testing improvements.

Had the tower channel muted 😅

Lost opportunity to kill ServiceBuilder for 0.5 and go all in on tuples implementing Layer x)

If ServiceBuilder is the only reason tower-http has a public dep on tower (not just the service & layer crates), I'd be strongly in favor of dropping ServiceBuilderExt

tender oar

Yeah also already a couple of Github issues.

dry spruce

Where?

tender oar

For my own tower derivative I kept the ServiceBuilder as I do like the basic shape of it, but I don’t have any kind of ServiceBuilderExt.

dry spruce

Ah but that issue is entirely unrelated to tower-http

@iron delta I think you should publish tower 0.5.1 w/ the fixed minimum dependency, as clearly multiple people are running into the bug 😦

tender oar

With tuple instead of service builder I suppose you mean (L1, L2, … Ln, S) as Service?

Never considered that. Interesting

dry spruce

Not as Service, as Layer

I added that a while back

tender oar
dry spruce

I see we're still using ServiceBuilder in axum docs.. Need to wait for David to get back to the project though, makes little sense for me to keep piling up PRs that nobody can merge 🥲

tender oar

True story

dry spruce
tender oar

Happy to do so, but given how conservative a lot in tower is I would like to hear those positive affirmations in the form of opinions first

Ah you said “check”. Ok I’ll do the report already. Need a coffee first

tender oar
dry spruce If you're interested in helping, it would be very cool if you could check whethe...

Ignoring dev-dependency purposes, tower is used as a dependency for the following two cases:

  • tower::util::Oneshot in follow_redirect: can be worked around, no biggie
  • ServiceBuilderExt, but if you get rid of ServiceBuilder concept that might as well be dropped

So yeah TLDR, you're spot on @dry spruce seems there is no real need for tower to be a dependency of tower-http once you take ServiceBuilder out of the picture. Except as a dev-dependency that is.

dry spruce

Oh, I was only talking about the use as a public dependency 🙂

The usage in follow_redirect is private, right?

tender oar

Yes

dry spruce

So there's probably no harm in keeping that

tender oar

But it's tiny utility doesn't justify for me personally its continued dependency 🤷‍♂️ But again, not up to me of course ^^

dry spruce

My justification for keeping it would be that people are going to have tower in their dep tree anyways

tender oar

Most likely, yes.

dry spruce

But OTOH, Oneshot seems like a particularily trivial one to replace

@zinc rune @hoary cargo @potent garden you're the first few people that come to mind that have contributed to tower-http more than once and are likely currently using it.
Wdyt about dropping the public dependency on tower (not tower-service and tower-layer, of course) by dropping ServiceBuilderExt?

zinc rune

I think the named methods provide a more readable configuration when actually used, so i feel it has value

Could it be done a better way? Probably also the case

dry spruce

My preferred way for a while has been to use the Layer impls for tuples, i.e. instead of ServiceBuilder::new().foo(foo_options).bar(), use (FooLayer::new(foo_options), BarLayer::new()). Are you aware this is a thing?

zinc rune

I've seen, ya

dry spruce

IME, on average it's a similar amount of imports and overall feels more consistent when you're mixing and matching upstream (tower / tower-http) and custom layers (for which you usually have no extension trait methods)

zinc rune

I don't always love Finagle, but this is a case where I agree with the translation: custom layers are possible, common options are easy to read

I find the mention of "layer" redundant and noisy when it's a bunch together

Could just be my personal taste

dry spruce

I also find it annoying, hence the tuple thing 🙂

zinc rune

I meant even in your example 🙂

dry spruce

w/ ServiceBuilder and little / no ServiceBuilderExts it was way too many layer / Layers 😄

Yeah I get what you mean

I wonder if the naming is just sub-optimal?

IME you want to name the layer types much more often than the service types, even if using ServiceBuilderExts

So maybe the better naming would be tower_http::cors::{Cors, CorsService} instead of tower_http::cors::{CorsLayer, Cors}?

Of course changing to that now is gonna be some churn...

Alternatively, we could have tower_http::service::Cors + tower_http::layer::Cors. This makes me want to have canonical type aliases ^^

zinc rune

/me nods

I think I'm fine with it for a single custom layer or two

But would expect that a builder (either a newly designed builder, an extension trait, or something else) would have a nice simple method .cors()

dry spruce

LayerExt x)

(way too much magic to me)

I don't have any ideas for a builder type that avoids the dependency issues we just had.

If tower-layer depended on tower-service, we could have tower-service define ServiceBuilder and tower-layer impl Layer for it

Alternatively, split ServiceBuilder into tower_service::ServiceBuilder and tower_layer::LayerBuilder

I would personally still stay away from it, but I guess that could work... In tower 0.6 😭

Maybe I'll end up forking this whole ecosystem once I gain a better understanding of the backpressure problem and why axum doesn't make use of it 🙊

Backpressure in tower-async

frozen pier

i'm sorry about going straight to 0.5—i didn't realize that people wanted to change the builder stuff. i should've stuck with my guns and pushed for an alpha release.

tender oar

to be fair I think there is a bigger lesson to be learned here. And that this might be just a symptompt of people being anxious of wanting to move tower in general further and further. Now it kinda came out like a vulcano I guess because of being pressure from being in limbo for so long already.

Or maybe I'm just a rambling maniac 🤷‍♂️ all is possible

frozen pier

i mean, this was me just realizing there were a few, useful things that were unreleased and i wanted to fix that, but i think having a big, whizz-bang release in 0.6 that revisits a bunch of stuff is probably warrented/reasonable

dry spruce

Honestly I don't think much harm was done

All the discussion above could have also happened if sbd. asked about the tower public dep on tower-http independently of a new release 🙂

tender oar
frozen pier

whoops, sorry about that

iron delta

sorry I was out of town

dry spruce

Doesn't look like it

iron delta

okay I can publish things, did we merge the PRs etc and tag stuff?

dry spruce

I don't think that happened either (can check..)

Raising min version was done

Bumping version of tower itself + tagging not

iron delta

I have a lot on my plate today

dry spruce

Not now but maybe later today / tomorrow

mellow ridge

Any chance of getting tower-http released?

shadow crater

@dry spruce I think you're the resident expert on tower-http at the moment... any reason we shouldn't release tower-http 0.5.2?

(or does it actually need a 0.6 release? not clear to me)

dry spruce

It needs 0.6.0 due to ServiceBuilderExt 😒

I've advocated for just removing that (to get rid of the public dep on tower) but Sean didn't like the idea

I guess there's no strong reason not to release 0.6.0 but I'd prefer if we could ship tower 0.5.1 first

I guess I can do the release prep for that now

zinc rune

It doesn't have to stay

I just felt the value it provides is nice versus all the noise of using layers manually

Another option is to make it a tower-05 cargo feature, and 06, etc

So it could work for various versions

Another option is to provide a separate Service Builder

Yet another is just depend on layer

Or even to just ignore me

zinc rune
shadow crater

I do agree, though, that tower_http::ServiceBuilderExt is nicer than manual layering

dry spruce

So we have ServiceBuilder05Ext, ServiceBuilder06Ext and so on

dry spruce

I can also do the tag

(once PR is merged)

shadow crater
high flower

Hi, about tower-http Bearer/Basic authorization, they use PartialEq between HeaderValue to verify, which opens to timing attacks (see https://timing.attacks.cr.yp.to/ for real world examples)
will you be willing to accept a PR using something like: https://crates.io/crates/constant_time_eq ? or do you think this should be used inside hyper's HeaderValue implementation? (maybe branch on header.sensitive?)

simple matrix
tender oar

Nothing about the Authorisation basic rfcs say the password has to be in plain text though. It’s fine if you send a hashed version of it.

And for the server side there are secure ways to keep this in memory within protected areas, and ensure the memory is never leaked/zeroed out. This is possible to do via https://docs.rs/tower-http/latest/tower_http/auth/async_require_authorization/index.html

The ‘basic’ fn is not something you’ll use in scenarios where those kind of concerns matter, as you are only checking 1 hardcoded user there… so that’s for internal / very niche use cases where it hardly if at all matters someone will do such an attack… especially given it’s basic authentication

Such authorizer however deserves a crate on itself as depending how many scenarios and hardware/platforms/environments you wish to support that can make for a pretty big crate on its own :p

Just doing that different kind of compare won’t cut it if you really are concerned about active target attackers

simple matrix
tender oar

Didn’t say it solved that. That first part was merely a response on the specific mention of plaintext passwords.

Https will ensure no sniffing can happen and for server side there is plenty you can do in your authorizer implementation, but I believe such a complete implementation deserves its own crate.

On top of that the server should probably also prevent to send an absurd amount of auth calls to the user.

Yada yada yada. Plenty of that and much more you can do, but seems a bit much for tower-http which isn’t really focused on authentication.

simple matrix

Yeah sending plaintext passwords isn't really a problem. And I agree that it feels wrong for such utility to be in tower-http. I personally think it would be better to deprecate it

dry spruce

Hey we're probably doing a breaking change release soon anyways

I'd be up for deprecating or removing the entire middleware tbh

Can sbd. open an issue about this?

shadow crater

hmm, wish we had an alias for people with tower publish privs... anyways

can someone publish tower 0.5.1? the release prep PR is merged and I cut a tag off of it, just needs to be published

alternatively, give me publish perms and I'll do it kekw

dry spruce

@iron delta ^^^^^

zinc rune
restive pawn

Sorry if this has been asked before, but what's the current plan to publish a version of tower-http that's compatible with tower 0.5?

dry spruce

I'd like to get CI fixed first, but it doesn't seem like there's a lot of maintainer time available. (I am spread pretty thin and I think the same applies to others with write access to the repo)

restive pawn
dry spruce

Probably not :/

dry spruce

Can sbd. with write access approve? @iron delta / @hoary cargo / @zinc rune

iron delta
dry spruce

Thx!

I also have write access so could have merged myself after approval, fwiw

iron delta
dry spruce

no problem

For releasing, I guess I'd just do a PR bumping the changelog and Cargo.toml, and ask here again?

dry spruce
iron delta
dry spruce

Yup

Been ages that I did this manually, the two other project where I have these, it's automated ^^

dry spruce
zinc rune

is main ready for breaking changes?

are these re-exported and so upgrading the version would be a breaking change?

dry spruce

Yes, main is ready for breaking changes (see top of readme), but also it's not a breaking chnage

(see cargo-public-api metadata in Cargo.tomls)

Thanks!

undone osprey
shadow crater

I'll take a look at this later

(from what I remember of the issue, it seemed fine to add so reviewing the PR should be quick I imagine)

undone osprey
undone osprey
undone osprey
zinc rune

sure, lemme just remind myself how we release this crate 🙈

zinc rune
undone osprey
zinc rune

Hm, not sure

Seems an action auto made those

iron delta
hollow dragon

You probably want to add a CI job that compiles on a no_std target to check that this is actually no std.

main marsh
hollow dragon

The attribute doesn’t prevent you from using std directly or via a dependency. It just disables importing it by default or via the standard prelude. Testing on a real target in CI helps find these problems now and in the future.

(We’re in the process of making ratatui mostly no_std right now, so finding these things out over the last week or two has been interesting)

tender oar
hollow dragon

Sounds like a good idea. It was driven by another dev, so I was mostly reviewing and asking dumb questions on PRs

There may be some unlinked PRs though.

But they can be found by searching no_std in the repo

tender oar

Intuitively I would think it will also require patching some upstream deps

For ratatui that is

hollow dragon

We’re not doing the terminals, just core, widgets and macros.

queen pollen
hollow dragon

We’d already gone through splitting out core / widgets / backends into separate crates. So that was reasonably easy

The idea behind it is that it opens up ratatui to microcontrollers, uefi, and wasm targets which can bring their own backend stuff

sly aurora
wraith ruin

Hey all I've recently open this PR would love any feedback 🙂

undone osprey
zinc rune

Thanks for all the reviews, and prepping the release ❤️

undone osprey
undone osprey
zinc rune

published

undone osprey

ci fix if anybody has a moment @zinc rune / @zenith pulsar
(not sure if you prefer to have another maintainer review this sort of thing)

ugh i hate yaml/github ci... fix coming