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

Add support to run unit tests in a multithreaded context #3221

Merged

Conversation

eduar-hte
Copy link
Contributor

@eduar-hte eduar-hte commented Aug 9, 2024

what

The unit_test program has been updated to accept the mtstress argument to run operator/transformation tests in a multithreaded context.

why

The goal is to detect if the operator/transformation fails in this context.

misc

  • In this mode, the test will be executed 5'000 times in 50 threads concurrently.
  • Allocation & initialization of the operator/transformation is performed once in the main thread, while the evaluation is executed in the threads.

references

This work is related to #3215, to check whether the optional mutex on the Pm operator to access acmp trees is needed.

This was introduced in commit 119a6fc & 7d786b3 likely because of issue #1573. This option is off by default and it's not clear whether it's necessary, as stated here.

@eduar-hte
Copy link
Contributor Author

All operator & transformation unit tests were executed using this feature and no issue was detected.

NOTE: This doesn't guarantee that there's no multithreading issue lurking in there, though.

@marcstern marcstern added the 3.x Related to ModSecurity version 3.x label Aug 12, 2024
- This is controlled by specifying the 'mtstress' argument when running
  `unit_test`.
- The goal is to detect if the operator/transformation  fails in this
  context.
- In this mode, the test will be executed 5'000 times in 50 threads
  concurrently.
- Allocation & initialization of the operator/transformation is
  performed once in the main thread, while the evaluation is executed in
  the threads.
  - This is consistent with the library's support for multithreading,
    where initialization and loading of rules is expected to run once.
    See issue owasp-modsecurity#3215.
Copy link

sonarcloud bot commented Aug 13, 2024

@airween airween merged commit c9af0c7 into owasp-modsecurity:v3/master Aug 14, 2024
49 checks passed
@airween
Copy link
Member

airween commented Aug 14, 2024

Thanks, @eduar-hte!

A reminder to me: 11 tests are failed, I have to check the reason.

@eduar-hte eduar-hte deleted the unittest-multithreaded branch August 14, 2024 12:31
@eduar-hte
Copy link
Contributor Author

eduar-hte commented Aug 14, 2024

A reminder to me: 11 tests are failed, I have to check the reason.

I think you're referring to these unit tests: test-cases/secrules-language-tests/transformations/phpArgsNames.json.

I remember looking into this while working on the Windows port (PR #3132), because when I run unit_tests manually they'd fail (they're not part of the make check test suite (test/test-suite.in) and that's why the builds don't fail.)

These tests were added to in owasp-modsecurity/secrules-language-tests@a3d4405 and are related to PR #2387, which I understand is a v3.1 feature.

@airween
Copy link
Member

airween commented Aug 14, 2024

I think you're referring to these unit tests: test-cases/secrules-language-tests/transformations/phpArgsNames.json.

yes, exactly.

I remember looking into this while working on the Windows port (PR #3132), because when I run unit_tests manually they'd fail (they're not part of the make check test suite (test/test-suite.in) and that's why the builds don't fail.)

yes, I also wondered why this haven't appeared yet

These tests were added to in owasp-modsecurity/secrules-language-tests@a3d4405 and are related to PR #2387, which I understand is a v3.1 feature.

ah, thanks - I hadn't time to review the history, but yes, this explains the behavior. Seems like the tests had already added but the transformation not. I think we can remove this test file.

Thanks for helping discovery the situation.

@eduar-hte
Copy link
Contributor Author

ah, thanks - I hadn't time to review the history, but yes, this explains the behavior. Seems like the tests had already added but the transformation not. I think we can remove this test file.

If you don't want to remove it, you could update the submodule reference in test/test-cases/secrules-language-tests to reference the previous commit (owasp-modsecurity/secrules-language-tests@d03f4c1), as the commit with the phpArgsNames tests is the latest commit in that repository.

@eduar-hte
Copy link
Contributor Author

ah, thanks - I hadn't time to review the history, but yes, this explains the behavior. Seems like the tests had already added but the transformation not. I think we can remove this test file.

Submitted PR owasp-modsecurity/secrules-language-tests#10 to remove the tests associated to this missing transformation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants