smol-rs / futures-lite

Futures, streams, and async I/O combinators.
Apache License 2.0
439 stars 25 forks source link

Race always chooses future1 #5

Closed philippwinkler closed 4 years ago

philippwinkler commented 4 years ago

Thanks for all your work on Smol and associated crates. I'm learning a lot from reading the code and enjoy using the small apis. While reading through I came across what I think is a bug in the implementation of race.

https://github.com/stjepang/futures-lite/blob/master/src/future.rs#L504-L517

        if fastrand::bool() {
            if let Poll::Ready(t) = this.future1.poll(cx) {
                return Poll::Ready(t);
            }
            if let Poll::Ready(t) = this.future2.poll(cx) {
                return Poll::Ready(t);
            }
        } else {
            if let Poll::Ready(t) = this.future1.poll(cx) {
                return Poll::Ready(t);
            }
            if let Poll::Ready(t) = this.future2.poll(cx) {
                return Poll::Ready(t);
            }

I might be missing a subtle point, but shouldn't that be:

        if fastrand::bool() {
            if let Poll::Ready(t) = this.future1.poll(cx) {
                return Poll::Ready(t);
            }
            if let Poll::Ready(t) = this.future2.poll(cx) {
                return Poll::Ready(t);
            }
        } else {
            if let Poll::Ready(t) = this.future2.poll(cx) {
                return Poll::Ready(t);
            }
            if let Poll::Ready(t) = this.future1.poll(cx) {
                return Poll::Ready(t);
            }

To switch the order the futures get polled in the else branch.

ghost commented 4 years ago

You're right, this is a bug :)

ghost commented 4 years ago

This should be fixed now.

philippwinkler commented 4 years ago

Great, thanks!