search

tgstation / rust-g

Rust based libraries for tgstation
MIT License
28 stars 100 forks source link

Replace flume with crossbeam in HTTP #131

Closed Mothblocks closed 1 year ago

Mothblocks commented 1 year ago

We're getting a really weird panic in this area of the code and it seems to be a result of flume unwinding.

This is a shot in the dark experiment to see if using the much more popular and battle-tested crossbeam will resolve this issue. Long term we need to agree on what panic hooks should do (exit DD? try to handle when an error is expected (like HTTP)? just return something and let DM code fail silently?) and get them in. If this PR actually should be merged because flume is erroneous, then we need to remove it completely in redis as well, so this is a draft.

AffectedArc07 commented 1 year ago

Dumb/unrelated question but would this solve the issue we have where we need dedicated start_http_client() and shutdown_http_client() methods to wipe out old http clients after a round between DD restarts? We had some issues with safety and bad memory accesses until we did this approach.

Relevant patch: https://github.com/ParadiseSS13/rust-g/blob/master/paradise-rust-g-patches/0005-Paradise-HTTP-Module-Tweaks.patch

Bizzonium commented 1 year ago

Also please check out these 3 commits, it could give some inspiration. https://github.com/ss220-space/rust-g-tg/compare/a4800676b265337ff4ade5534c6692c678e7579a...fcc86bc4b0432d7d41bf86c3309e119b3c01db81

Mothblocks commented 1 year ago

This didn't actually fix it and the stack traces we're getting are the same. I'm going to get my panic hooks PR in better shape and get a better understanding