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

Remove unnecessary time.sleep(5) call in ramp-requests.py #659

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kdgyun
Copy link

@kdgyun kdgyun commented Oct 17, 2023

Background

In the ramp-requests.py file, there's no clear reason for using sleep(5) after executing the subprocess.run() method for an attack.
My guess is that when creating the process with run(), it was assumed to be non-blocking, and sleep(5) was given to match -duration 5s.
If this assumption is correct, the subprocess.run() method is blocking, and it won't execute the next command until the child process finishes, making the sleep unnecessary.
If there was an intention to add an arbitrary delay, there should be a specific comment or explanation. Otherwise, if someone assumes that subprocess.run() is non-blocking and adds time.sleep(5), it can cause confusion.

You can find more details about the blocking nature of subprocess in the following links:

Checklist

  • Git commit messages conform to community standards.
  • Each Git commit represents meaningful milestones or atomic units of work.
  • Changed or added code is covered by appropriate tests.

@kdgyun kdgyun requested a review from tsenart as a code owner October 17, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant