Diggsey / act-zero

Apache License 2.0
122 stars 7 forks source link

Drop the value manually, runs to completion #7

Closed mkj closed 3 years ago

mkj commented 3 years ago

In my testing this fixes the problem of #6, though I'm not sure if there would be other side-effects of the change

Diggsey commented 3 years ago

Thanks for the PR!

I believe this drop order is implicitly determined by the order of arguments here: https://github.com/Diggsey/act-zero/blob/9959ff6ddc3d5bebaacef3c2a6d860e87db31a4a/src/addr.rs#L20-L24

(Arguments are dropped in reverse order)

Could you test whether re-ordering these parameters also fixes the problem: if so I'd prefer that solution (with a comment in the code so that the order is not accidentally changed again) because it will have consistent order even if there's a panic.

Also, it would be good if you could add a test for this drop order to the two runtimes: https://github.com/Diggsey/act-zero/blob/f014319edc7d14a3b0900ed51b525634dc63bc65/src/runtimes/tokio.rs https://github.com/Diggsey/act-zero/blob/f014319edc7d14a3b0900ed51b525634dc63bc65/src/runtimes/async_std.rs

Diggsey commented 3 years ago

On second thoughts, it's the futs variable whose order we care about in relation to the value variable, so re-ordering the parameters won't help.

Instead, the value parameter should be re-bound to a variable after futs has been declared:

let mut futs = FuturesUnordered::new();
let mut value = value;

This should ensure the correct drop order.

mkj commented 3 years ago

Thanks, re-binding value works. I'll push a test tomorrow. Strangely tokio didn't have any problem before.

mkj commented 3 years ago

Pushed with tests. The tokio one would have failed before the fix if running threaded, but by default it's one thread so it won't race.

Diggsey commented 3 years ago

Thanks!