Fix tests that leak threads (pytest 8)

[ecerulm]

Bumping the pytest version to 8.0.2 revealed that some tests are leaking threads. The tests create a SocketServerThread and those threads never finish. These unterminated threads confuse pytest-memray since while running a test one of these threads can do an allocation that will be counted as part of this test.

In particular the following tests currently do not properly terminate the threads:

• test_socket_close_socket_then_file
• test_socket_close_stays_open_with_makefile_open
• test_close_after_handshake
• test_total_timeout
• test_socket_close_socket_then_file
• test_socket_close_stays_open_with_makefile_open
• test_multipart_assert_header_parsing_no_defects
• test_chunked_specified[bytes-POST-False] (which is SKIPPED but it start the server thread, then skips itself using pytest.skip())
• test_chunked_specified[bytes-PUT-False] (which is SKIPPED but it start the server thread, then skips itself using pytest.skip())
• test_chunked_specified[bytes-PATCH-False] (which is SKIPPED but it start the server thread, then skips itself using pytest.skip())

This PR addresses the problem by

• waiting after each test for the thread to finish, if it does not finish it time, generate an exception. Silently continuing only hides the problem.
• introducing a quit_event , on the teardown_method we signal the quit_event and the socket_handlers can read it, and acts appropriately.
  • this unfortunately complicates the code since now we can't just block indefinitely, we need to timeout, to check for the quit_event and terminate in that case.
• introduces a new pytest marker server_threads to allow to test funcionality. I can remove this marker after but right now you can test this with nox -rs test-3.10 -- -m server_threads

Closes #3335

Alternatives

• Ask pytest-memray to give an option to only count the allocations made on the current test thread (and not on the server threads). See option to count only memory used in current thread / limit_memory("10MB", current_thread_only=True), bloomberg/pytest-memray#111
• Increase the limit to @pytest.mark.limit_memory("10.10 MB"). This does not fix the problem but it's way easier and less convoluted. But ideally we should not leave threads lingering from test to test.
• Make sure each server threads dies of natural causes. Most of the server thread close down gracefully since the code path servers a single request and then returns. But the tests mentioned above they are not doing that.
  • some of the tests like test_socket_close_socket_then_file I believe they really need a outside signal
  • some test like * test_chunked_specified[bytes-POST-False], should not even create a server thread in the first place, the whole test can be SKIPPED before self._start_server(socket_handler).

[pquentin]

Thanks for this work! My main worry is the reliability of the time-based logic. You would expect LONG_TIMEOUT * 3 to be enough, but CI always surprises me in this regard. We'll have to be ready to bump that constant if needed. Thankfully bumping it will not make the happy path slower.

[ecerulm]

Thanks for this work! My main worry is the reliability of the time-based logic. You would expect LONG_TIMEOUT * 3 to be enough, but CI always surprises me in this regard. We'll have to be ready to bump that constant if needed. =

I've increased it to LONG_TIMEOUT *2 + 5.0 , in principle the maximum time that the thread can take to notice the quit_event is LONG_TIMEOUT and the thread should terminate immediately (0.1s) after that. If it takes more than 5 seconds something must be wrong.

Thankfully bumping it will not make the happy path slower.
yeah, the if the thread actually terminates fast the .join() will also return fast. It won't make the happy path slower.

[ecerulm]

I've removed the print statements, extraneous, comments, and the unnecessary time.sleep()

[illia-v]

LGTM, thanks for this!

Thanks for this work! My main worry is the reliability of the time-based logic. You would expect LONG_TIMEOUT * 3 to be enough, but CI always surprises me in this regard. We'll have to be ready to bump that constant if needed. =

I've increased it to LONG_TIMEOUT *2 + 5.0 , in principle the maximum time that the thread can take to notice the quit_event is LONG_TIMEOUT and the thread should terminate immediately (0.1s) after that. If it takes more than 5 seconds something must be wrong.

What do you think about adding a comment to the code describing why LONG_TIMEOUT * 2 + 5.0 was chosen?

• introduces a new pytest marker server_threads to allow to test funcionality. I can remove this marker after but right now you can test this with nox -rs test-3.10 -- -m server_threads

I'd drop server_threads before the PR is merged if there is no expected use for the marker.

[ecerulm]

What do you think about adding a comment to the code describing why LONG_TIMEOUT * 2 + 5.0 was chosen?

Done, in latest version

I'd drop server_threads before the PR is merged if there is no expected use for the marker.

Done

[illia-v]

I'll merge this to include the changes in a base for a #3366 fix