Leak waiters on cancellation

[singggum3b]

Hi, i tried to report on closed issue but no response so i'm open a new one.
Long story short :
Pool hang after some last accquire call but get cancelled. Two scenario:

• pushed waiter but get cancelled so 1 extra waiter
• got woken but then cancelled.

-> Due to the pool rely on connection release to trigger wakeup call.

acquire function might be a source of leak. When calling acquire on a pool and waiter is pushed to the list, but then the acquire future is then dropped before the timeout, then the waiter is stuck in the list indefinitely.

Originally posted by @singggum3b in #83 (comment)

[abonander]

The waiters list probably needs to be an atomic linked list so that an individual waiter can remove itself on-drop.

[abonander]

In fact, I wonder if the waiters list can be implemented without additional allocations, using the stack- or heap-pinned futures themselves as storage.

I realize in the general case that dropping cannot be relied upon for safety, but in the case of a pinned future, I'm not sure it's possible to leak one in safe code without it also remaining a valid memory location for the life of the program: a stack-pinned future cannot be moved into mem::forget() or an Rc cycle, and a heap-pinned future if leaked will be valid for the rest of the program's life.

The only possible situation I can think of is where a thread aborts without unwinding its stack or aborting the program as a whole. There's pthread_exit and Windows has TerminateThread but neither is recommended for use in the general case (and both require unsafe to invoke).

@mehcode, thoughts? We don't have to do this at first or at all but it sounds like something interesting to experiment with.

[abonander]

In general, can we assume that a pinned value will either have its destructor called or have its memory location remain valid for the life of the program?

In fact, the answer appears to be yes, and just above this is an example of exactly what I'm talking about. Awesome.

[singggum3b]

I think it's possible to delegate the pool logic to a more focused library e.g future-intrusive ?
https://docs.rs/futures-intrusive/0.3.1/futures_intrusive/sync/struct.GenericSemaphore.html

[abonander]

The semaphore from future-intrusive doesn't give owned permit guards so PoolConnection would have to have a lifetime tied to the Pool it came from. This doesn't seem like a deal breaker at first but it would cause issues for the design of PgListener which wants to own a Pool and a PoolConnection at the same time, and in general I don't want to introduce an API restriction just to accommodate a dependency.

I'm working on a better core implementation of Pool that we want to publish as a separate crate for use in other projects. I'll start experimenting with it in sqlx very soon.

[singggum3b]

Hi @abonander , i understand your point.
I just hope that if possible we should concentrate the ecosystem, while rust still young and has not many expert about the language like you. May be work together with author of future-intrusive ?
Anyway it's just my thought ;)
Great library and hope rust will have more fundamental way to lift the RAII burden for us programmer !

[abonander]

Fixed by #918