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

HelpCommand implementation subject to race conditions. #2123

Closed
mikeshardmind opened this issue Apr 29, 2019 · 1 comment

Comments

@mikeshardmind
Copy link
Contributor

commented Apr 29, 2019

Summary

By using a single instance of a help command and doing preparation work (including assigning of context) to said single instance while also relying on that in other places , the help formatter is subject to race conditions.

Reproduction Steps

Have a bot under heavy load such that if 2 help commands are issued at the same time, it's possible for results to not end up truly FIFO due to channel based ratelimits. and context switching with asyncio

Expected Results

All prep work would be kept separate per invoke, including keeping context correct.

Actual Results

Checklist

  • I have searched the open issues for duplicates.
  • I have shown the entire traceback, if possible.
  • I have removed my token from display, if visible.

System Information

  • discord.py version: 1.0.1
  • Python version: 3.7.2
  • Operating system: Windows, for testing, but can confirm this is not system specific.

@Rapptz Rapptz added the no repro label Apr 29, 2019

@Rapptz

This comment has been minimized.

Copy link
Owner

commented Apr 29, 2019

Personally I haven't experience this issue in both the original HelpFormatter and the new HelpCommand. Both HelpFormatter and HelpCommand were single-instances in their design. However I understand that it is possible on a theoretical basis. There are a few ways to fix it as far as I'm concerned:

  1. Use contextvars to fix the context issue.
    • This opens up another problem, in that every other attribute is no longer safe.
    • Contextvars requires 3.6+ to be supported while the lib is currently 3.5.3+ due to dependencies so it'd be breaking.
  2. Pass the Context around in every callable that needs it.
    • Again, this fixes the underlying problem with the Context but not with every other attribute.
    • This is a rather large breaking change for consumers.
  3. Copy the instance when necessary and run the functions on that copy.
    • This one fixes the above issues but it's still a breaking change if someone expected the instances to maintain state throughout the whole ordeal. However, it's probably not as big of a breaking change as the other two solutions.

Personally if I were to fix this I'd use the 3rd solution and swallow the minor breakage that was not explicitly documented anywhere.

@Rapptz Rapptz closed this in ad5beed Apr 29, 2019

@Rapptz Rapptz added bug and removed no repro labels May 10, 2019

@Rapptz Rapptz added this to the v1.1 milestone May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.