Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace flume with crossbeam in HTTP #131

Closed
wants to merge 2 commits into from

Conversation

Mothblocks
Copy link
Member

@Mothblocks Mothblocks commented May 13, 2023

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
Copy link
Member

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
Copy link

Also please check out these 3 commits, it could give some inspiration.
ss220-space/rust-g-tg@a480067...fcc86bc

@Mothblocks
Copy link
Member Author

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

@Mothblocks Mothblocks closed this May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants