GeezerVison is showing github.com — gh-94609: Make test_ssl.ThreadedEchoServer exceptions appear in a main thread by arhadthedev · Pull Request #92475 · python/cpython
Conversation
arhadthedev
commented
May 8, 2022
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
…t breaks it with a long-live thread pool
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
commented
Jul 6, 2022
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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)
|
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This PR is stale because it has been open for 30 days with no activity.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Closing; I suspect the test module changed too much last four years so it would be easier to redo it from scratch.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|