Skip to content

Rewrote http.py to be faster - #68

Merged
fzipi merged 2 commits into
coreruleset:mainfrom
theseion:performance-improvement-http
Mar 29, 2022
Merged

Rewrote http.py to be faster#68
fzipi merged 2 commits into
coreruleset:mainfrom
theseion:performance-improvement-http

Conversation

@theseion

@theseion theseion commented Mar 4, 2022

Copy link
Copy Markdown
Contributor

…, mainly by using select() instead of timeouts

Split from PR #66, part 1

@theseion
theseion requested review from fgsch and fzipi March 4, 2022 07:42
theseion added a commit to theseion/ftw that referenced this pull request Mar 4, 2022
theseion added a commit to theseion/ftw that referenced this pull request Mar 4, 2022
This was referenced Mar 4, 2022

@fzipi fzipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm...

  • requirements.txt updates should be in the dependency updates only.
  • same with the test updates, should be in the tests only

@theseion

theseion commented Mar 6, 2022

Copy link
Copy Markdown
Contributor Author

I did that so the PR checks would run through and to show, that the change doesn't affect the tests. Do you still want me to change it?

@fzipi

fzipi commented Mar 6, 2022

Copy link
Copy Markdown
Member

Are the tests failing when upgrading dependencies?

@theseion

theseion commented Mar 6, 2022

Copy link
Copy Markdown
Contributor Author

Yes, partly because the domains used in the tests are unreliable.

@fzipi

fzipi commented Mar 7, 2022

Copy link
Copy Markdown
Member

Well, then we should change the description on this PR to match whatever is doing. Also, if someone needs to revert it will erase both the http.py rewriting AND the tests update.

I think that the easiest way to solve this is to create a new PR for updating the tests and the reason of reliability. We merge that one. Then rebase this one so it doesn't update tests also. We merge this one.

Then rebase the following and keep going :)

@theseion

theseion commented Mar 7, 2022

Copy link
Copy Markdown
Contributor Author

That PR already exists :) Your point about reverting is a good one. I'll update the PR's and indicate the proper order in which to review them.

@theseion
theseion force-pushed the performance-improvement-http branch from 495c16e to 5509b0e Compare March 10, 2022 20:26
@theseion

Copy link
Copy Markdown
Contributor Author

@fzipi @fgsch Final one.

Comment thread ftw/http.py Outdated

@fzipi fzipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @fgsch What do you think?

@fzipi

fzipi commented Mar 29, 2022

Copy link
Copy Markdown
Member

Merging.

@fzipi
fzipi merged commit 10a82e6 into coreruleset:main Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants