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

Performance improvement #66

Closed
wants to merge 19 commits into from

Conversation

theseion
Copy link
Contributor

@theseion theseion commented Nov 9, 2021

  • Reworked http module to run tests much faster by removing most of the timeouts.
  • Updated dependencies
  • Added Python 3.10
  • Fixed a couple of deprecations

@theseion theseion requested a review from fzipi November 9, 2021 20:16
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Wow @theseion I love what I am seing here! So moving to select gives us probably way better efficiency. And only reading time if we are going to use it, nice catch. I wonder why the time.sleep was being used, maybe @fgsch has better idea. Also, he might know more if this might affect upstream users.

Other questions:

  • Do we need to update documentation to reflect on these changes?
  • Is there any additional test we should consider on adding, just to be safe?

@dune73
Copy link
Member

dune73 commented Nov 15, 2021

Cool, @theseion.

For the record, we still plan to move to go-ftw with the project, so this is simply cool, nice2have and good for ftw, but without long term impact as far as CRS is concerned. Correct?

@theseion
Copy link
Contributor Author

For the record, we still plan to move to go-ftw with the project, so this is simply cool, nice2have and good for ftw, but without long term impact as far as CRS is concerned. Correct?

That is correct. It's mainly for me, so I can run tests for the changes I'm making to the regexp-assemble stuff.

@dune73
Copy link
Member

dune73 commented Nov 15, 2021

Thank you. It's very nice to PR these fixes nevertheless.

@theseion theseion marked this pull request as ready for review January 30, 2022 12:08
@theseion
Copy link
Contributor Author

I have this version working now with the CRS test suite. The tests for Apache all pass. The tests for NGiNX don't but at least the failures are now stable (with the current setup we have random failures).

I've also taken the opportunity to update dependencies but that can easily be split into a separate pull request if necessary.

Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

LGTM. As there is a small change for the Windows platform, should we fire tests using Windows also?

@theseion
Copy link
Contributor Author

theseion commented Feb 5, 2022

Yes, that's probably a good idea. Do you have it set up on a Windows machine?

@theseion
Copy link
Contributor Author

theseion commented Feb 5, 2022

I ran the tests on my Windows machine (Windows 11) in Powershell. There's one test that is flaky but it's also flaky on macOS (example.com sometimes returns 404 instead of 200 for "Test multipart using list"). Otherwise, everything looks good.

@fzipi
Copy link
Member

fzipi commented Feb 12, 2022

@fgsch I think this one is ready. Do you have any comments here?

ftw/http.py Outdated Show resolved Hide resolved
ftw/http.py Outdated Show resolved Hide resolved
@fgsch
Copy link
Contributor

fgsch commented Mar 2, 2022

👋 I'm adding comments as I go through this PR (so far non-blockers).
It'd be great if you could split this PR into multiple smaller ones so it's easier to review 🙏

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
Added test build for Python 3.10

Split from PR coreruleset#66, part 2
This was referenced Mar 4, 2022
@theseion
Copy link
Contributor Author

theseion commented Mar 4, 2022

This PR has been replaced by #68, #69, #70.

@theseion theseion closed this Mar 4, 2022
@theseion theseion deleted the performance-improvement branch March 4, 2022 08:09
theseion added a commit to theseion/ftw that referenced this pull request Mar 9, 2022
Added test build for Python 3.10

Split from PR coreruleset#66, part 2
theseion added a commit to theseion/ftw that referenced this pull request Mar 10, 2022
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.

4 participants