| msg205306 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2013-12-05 15:20 |
| Accoring to LCOV thread_PyThread_interrupt_main() is never called by any test. It's only referenced by some idle code. http://tiran.bitbucket.org/python-lcov/Modules/_threadmodule.c.gcov.html#1110 |
|
|
| msg280047 - (view) |
Author: Charlie Proctor (charlie.proctor) * |
Date: 2016-11-04 14:37 |
| Found this through the "Random Issue" button -- I've uploaded a simple test case for thread.interrupt_main(). This is my first patch :) So let me know if there's something else I should do and I'd love to hear any feedback... |
|
|
| msg280050 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2016-11-04 15:07 |
| Thanks, Charlie. You should use addCleanup to handle the resetting of the state, so that it gets cleaned up no matter what happens in the test. IMO the comments should either be omitted or be more descriptive about what exactly is being tested. For example "If this timer goes off, then interrupt_main did not work, so fail the test". I don't really understand what exactly is being tested in the body...it looks like two tests, one for calling it from the main thread (I suppose it makes sense to test that, but I don't know what behavior is expedted) and one from a subthread, which I would think was the real test. I would expect the main thread to be catching KeyboardInterrupt, based on the description of interrupt_main, so I'm not even sure what the sigalrm is for. Can you explain? |
|
|
| msg280054 - (view) |
Author: Charlie Proctor (charlie.proctor) * |
Date: 2016-11-04 16:02 |
| Thanks for the feedback David! I've posted a revised patch with more descriptive comments and the restoration code moved into addCleanup. As described in the comments, in the context of this test, the "main" thread is the one running the test suite... so rather than self.assertRaises(KeyboardInterrupt), I assert that appropriate SIGINTS are received. The lock is used to facilitate these assertions, since signals are asynchronous. |
|
|
| msg280055 - (view) |
Author: Charlie Proctor (charlie.proctor) * |
Date: 2016-11-04 16:06 |
| To clarify further, the SIGALRM handler catches the timeout sent by the ITIMER_REAL... whereas the SIGINT handler catches the interrupt_main() signals. |
|
|
| msg280060 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2016-11-04 16:46 |
| Ah, of course. The revised comments look good. I think I'd rather see the two cases tested separately, but if they are kept as one another comment ("lock was successfully released; reacquire the lock and test that it also works from a sub-thread") might be helpful. |
|
|
| msg280066 - (view) |
Author: Charlie Proctor (charlie.proctor) * |
Date: 2016-11-04 18:39 |
| I broke the two cases (interrupt_main from test thread and from sub-thread) into two separate tests, using an "expect_sigint" helper. Let me know what you think -- thanks! |
|
|
| msg280081 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2016-11-04 20:44 |
| This looks great, thanks. |
|
|
| msg404588 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2021-10-21 12:02 |
| Tests have been added as part of GH-24755. |
|
|