Drop dependency on futures-intrusive

[paolobarbolini]

The crate seems to be unmaintained and unsound:

• Does this crate alias mutable references? Matthias247/futures-intrusive#56
• NoopLock is unsound Matthias247/futures-intrusive#42

Alternatives:

• Use tokio Semaphores
  • Always (I personally wouldn't mind, I'm not a fan of async-std)
  • Find an alternative for async-std users (might complicate the implementation)

[abonander]

There's async-lock which would be an okay replacement, although the current implementation of Pool requires being able to manually release permits to the semaphore which neither async_lock::Semaphore nor tokio::sync::Semaphore supports.

It wouldn't be super difficult to fix that, as it's just a workaround for the fact that we need an Arc-backed guard but want to store more than just the semaphore itself in that Arc, but none of these crates support that kind of API. It just feels really dumb to have the pool's SharedState in an Arc and then inside of that have an Arc<Semaphore>.

Dropping async-std is an orthogonal proposal which I've been meaning to draft. It would simplify a lot, but if people are actually still using it, I don't really want to kick them to the curb. There's async-compat of course but that would be a significant refactor on the user's end.

[abonander]

I guess the SharedState struct could be #[repr(C)] and then if the semaphore is the first field we could potentially convert Arc<SharedState> to Arc<Semaphore>, but the documentation for Arc::from_raw() only allows that if the source and the destination type are the same alignment and size.

[abonander]

We could also just roll our own semaphore, basically just re-implement async_lock::Semaphore using the crate it's built on, event-listener. That was the part that was easiest to get wrong anyway, which was the waiter queue.

[paolobarbolini]

What I was thinking with tokio is that if their Semaphores were to work as a replacement to futures-intrusive we could always import it, even when async-std is enabled. This doesn't mean that we break support for async-std. because tokio Semaphores don't depend on the tokio runtime (AFAIK), it's just that we would be treating async-std as a second class citizen and independently of which runtime gets enabled tokio would always be in the dependency tree because of it's runtime independent APIs.

EDIT: never having been interested in async-std I don't mind the nuclear option of ripping it out though

[abonander]

Getting rid of async-std would simplify a lot of things so I've opened #1669 to discuss it.

[Darksonn]

being able to manually release permits to the semaphore which neither async_lock::Semaphore nor tokio::sync::Semaphore supports.

Actually, tokio::sync::Semaphore does support this. The method is here.

[abonander]

Ah, I scanned right past that method because I was looking for one with release in the name.

[abonander]

If we keep async-std support, however, there is potential pushback from requiring Tokio, since importing it just for tokio::sync::Semaphore would still bring along a lot of unused code: #1669 (comment)

[carllerche]

@abonander After acquiring a semaphore permit, you can forget it. Then call add_permit to add it back. The API is aimed to push you towards RAII, but you can go "manual" if needed.

Also, if you find API gaps, please report them. We are always happy to fill those gaps when possible.

[Matthias247]

As the author of futures-intrusive, I can tell you that its pretty much the same as tokio's implementation. Both will use intrusive linked lists, and apparently there's some theoretical issues with those which don't manifest itself anywhere in pratice.

It apparently works fine for you in pratice, so I don't think there's any downside in keeping it. However if you want to go tokio-only anyway and reduce dependencies, it probably makes sense. async-lock might have more allocations in the hot path and different fairness guarantees than the other 2 impleemntations.

[abonander]

Yeah, for the record, Tokio appears to have the same soundness issue: tokio-rs/tokio#3399

NoopMutex is an orthogonal concern, but we don't utilize that.

[abonander]

Since the underlying soundness concern seems to be just a bad interaction between how async works and LLVM's noalias, I'm going to close this as it's not really something we can fix without avoiding self-referential types entirely.

[abonander]

@Matthias247 I am somewhat concerned, however, about the maintenance status of futures-intrusive. It seems like you might have been gone for a while. Are you willing to continue maintaining futures-intrusive, and, particularly, adopt a solution for the above if and/or when Rust provides one?

I myself have a number of crates that I've just kind of forgotten about and haven't had the energy to maintain, so I understand if that's where you're coming from, but since SQLx is deployed in mission-critical applications I don't want to have a bit-rotting dependency to worry about.

[FinnRG]

@abonander What is your current stance on dropping futures-intrusive? The crate currently still depends on version 0.11 of parking_lot, which causes duplicate dependencies if used with other popular libraries such as axum. An update was merged with Matthias247/futures-intrusive#62, but @Matthias247 hasn't yet published a new patch release, even after several pings.

[antonio-hickey]

@abonander Hello fren, any updates on this? I'm getting dinged with criticals from RustSec advisory about the futures-intrusive crate.