search

emmett-framework / granian

A Rust HTTP server for Python applications
BSD 3-Clause "New" or "Revised" License
2.67k stars 79 forks source link

Set error result on awaitables when cancelling futures #279

Closed sciyoshi closed 5 months ago

sciyoshi commented 5 months ago

This seems to fix #252 and possibly #276 as well.

sciyoshi commented 5 months ago

Nevermind, I still see a shutdown issue happening with my main test case, although this fixes the simple repro provided in https://github.com/emmett-framework/granian/issues/252#issuecomment-2064512425.

gi0baro commented 5 months ago

@sciyoshi I think this doesn't solve anything: probably it seems to fix #252 just because the overall behaviour is flaky. In fact, I'm quite sure this is actually wrong, as the cancellation output is already covered by https://github.com/emmett-framework/granian/blob/master/src/callbacks.rs#L224 and https://github.com/emmett-framework/granian/blob/master/src/callbacks.rs#L237, which is set on the internal state directly by Python here https://github.com/emmett-framework/granian/blob/master/src/callbacks.rs#L202

gi0baro commented 5 months ago

@sciyoshi maybe the actual issue might be with this line https://github.com/emmett-framework/granian/blob/master/src/callbacks.rs#L120, as it might overwrite the cancelled state. Probably a more error-prone strategy would be to check the state value before setting the completed one.

sciyoshi commented 5 months ago

Fair point, it might be that the state isn't getting set correctly.