Skip to content

gh-94609: Make test_ssl.ThreadedEchoServer exceptions appear in a main thread #92475

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

Open
wants to merge 92 commits into
base: main
Choose a base branch
from

Conversation

arhadthedev
Copy link
Member

@arhadthedev arhadthedev commented May 8, 2022

Feature or enhancement

This PR:

  • adds test.test_ssl.Server that is like test.test_ssl.ThreadedEchoServer but (1) implemented as a context manager and (2) replaces plain printing of server-side tracebacks with bubbling the exception up between threads through a future. The (2) is implemented as follows:
    • the server force-closes a client connection when exceptions occur so a client thread instantly gets ConnectionAbortedError / ConnectionResetError hence knowing that the future is going to have a server exception inside
    • the client-side context manager gets the connection failure and rethrows the prepared server exception from the future
  • converts some test cases into using the new approach; others will be converted later in separate PRs
  • enforses a timeout for all socket operations using setUp/tearDown
  • teaches tests to push empty packets because TLS 1.3 delays the handshake until some data are sent

Pitch

  • expected exceptions (caused by *_fail test cases) do not flood a console with unnecessary stack traces (and a heart of a watcher with fear)
  • instead, they can be captured by assertRaises in a main thread; unexpected exceptions will migrate into a Tests Result report footnote failing the run so no need to scroll the log hundreds lines up
  • a server-side exception won't hang a main thread until a socket reading timeout happens
  • replacement of test_ssl and test_asyncio threaded servers with a unified implementation (they cannot be ported to asyncio because a client part is blocking and requires thread separation anyway)

TODO

Plans for future PRs:

  • port all test cases to the new class
  • remove the original ThreadedEchoServer
  • port other test suits that use multithreaded servers (including the ones that currently use asyncore like test_ftp and test_logging)

Instead of threads and flags, employ high-level concurrency primitives
like tasks and futures.

A thread pool allows to run tests in parallel so it is future-proof.
Start delegating all exceptions to a main, tester thread.

- thread creation for each new client has no use in single-shot
  disposable servers that need to be created fresh for every test case
  anyway

- stop-flag has no use either; there is no interactive session as well
  as waiting for other users. To test server-side connection abruption,
  instruct a server instance in advance

- wrap_conn either modified self.sslconn and returned True, or swallowed
  exceptions and returned False. Now it either return sslconn directly
  or lets an exception to bubble up through a future into a main thread.
  The main thread, in its turn, either expects it with assertRaises() or
  lets it bubble further so the test case is reported as failed

- transparent bubbling up means no explicit collection of error
  messages in conn_errors. Let unittest package do this job for us

- universal read-write-close operations are changed from inner ifs to
  assignment of specific lambda functions

- constant repetition of `if support.verbose and chatty:` //
  `sys.stdout.write(' server: ...\n')` is moved into log()

- chatty and connectionchatty flags are merged to form a single story of
  communication between a client and a server

- the first usage of ThreadedEchoServer is on line 1286 while the server
  itself was declared starting with line 2557; the new implementation
  is put before the first usage for sequential narration
In ThreadedEchoServer chatty=True by default so omit it.
@arhadthedev arhadthedev force-pushed the threadedserver branch 2 times, most recently from 1ef0c2c to 610e8e1 Compare May 19, 2022 09:44
Force clients to wait on a server socket while the server worker thread
starts up, instead of erroring on unavailable host. For this, open
the socket in a main thread.

Server socket timeout has no use because the only operation it limits
is listen(). To have it permawaiting we need a main thread interrupted
without terminating the whole program; that's possible under a debugger
only.
@arhadthedev arhadthedev changed the title Make test_ssl.ThreadedEchoServer exceptions appear in a main thread gh-94609: Make test_ssl.ThreadedEchoServer exceptions appear in a main thread Jul 6, 2022
@arhadthedev
Copy link
Member Author

arhadthedev commented Jul 6, 2022

A thread pool triggers ENV CHANGED. Any ideas how to remedy this without creating&destroying a loop for each individual test?

== Tests result: ENV CHANGED ==
425 tests OK.
10 slowest tests:
- test_tools: 5 min 45 sec
- test_concurrent_futures: 2 min 54 sec
- test_multiprocessing_spawn: 2 min 40 sec
- test_gdb: 2 min 7 sec
- test_multiprocessing_forkserver: 1 min 28 sec
- test_asyncio: 1 min 27 sec
- test_venv: 1 min 17 sec
- test_multiprocessing_fork: 1 min 17 sec
- test_signal: 1 min 5 sec
- test_regrtest: 50.5 sec
1 test altered the execution environment:
    test_ssl
10 tests skipped:
    test_devpoll test_ioctl test_kqueue test_launcher test_msilib
    test_startfile test_winconsoleio test_winreg test_winsound
    test_zipfile64
Total duration: 11 min 1 sec
Tests result: ENV CHANGED
make: *** [Makefile:1863: buildbottest] Error 3
Error: Process completed with exit code 2.

@arhadthedev arhadthedev marked this pull request as ready for review July 6, 2022 19:23
@arhadthedev
Copy link
Member Author

Any ideas how to remedy this without creating&destroying a loop for each individual test?

Not relevant anymore. The creationa and destruction happens for some modules only.

Whatever, it's ready for a review.

@arhadthedev arhadthedev marked this pull request as ready for review July 10, 2022 16:14
@arhadthedev
Copy link
Member Author

Everything is green and ready for a review (after the release blocker crunch is over, of course).

@arhadthedev
Copy link
Member Author

@tiran Would you mind to look at this PR? It adds a unified separate-threaded test.support.threading_helper.Server that I'm planning to use as a foundation to replace asynchat-based servers where asyncio is not applicable.

Comment on lines +17 to +25
def _release():
global _thread_pool
_thread_pool = None


def init():
global _thread_pool
_thread_pool = ThreadPoolExecutor()
unittest.addModuleCleanup(_release)
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added init() to avoid a modified environment test failure due to a leaked object. If there's a better solution, I'll use that instead.

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