Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 30, 2020

This avoids the seemingly infinite backtraces that get generated
when interrupting the parallel testsuite.

This avoids the seemingly infinite backtraces that get generated
when interrupting the parallel testsuite.
@sbc100 sbc100 requested a review from jgravelle-google May 30, 2020 02:46
@sbc100
Copy link
Collaborator Author

sbc100 commented May 30, 2020

Finally figured it out.



def g_testing_thread(work_queue, result_queue, temp_dir):
signal.signal(signal.SIGINT, signal.SIG_IGN)
Copy link
Member

Choose a reason for hiding this comment

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

maybe a comment here? I'm not sure what this does or how it helps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't speak signal

self.init_processes(test_queue)
results = self.collect_results()
try:
self.init_processes(test_queue)
Copy link
Member

Choose a reason for hiding this comment

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

does this line need to be inside the try?

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably if init-ing fails we might need to clean up... forget what it does and if/how it can fail though.

self.init_processes(test_queue)
results = self.collect_results()
finally:
for p in self.processes:
Copy link
Member

Choose a reason for hiding this comment

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

maybe a comment here too, why this is needed?

for p in self.processes:
p.terminate()
p.join()
self.processes.clear()
Copy link
Member

Choose a reason for hiding this comment

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

what does .clear() do that is different from self.processes = []? (is it not a normal array somehow?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the latter allocates a new array object/id?
Which could potentially cause problems if other objects are holding references to self.processes specifically.

If so: that's comment-worthy, cause it's a nonlocal concern.

Copy link
Contributor

@jgravelle-google jgravelle-google left a comment

Choose a reason for hiding this comment

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

Not entirely sure how this works but seems ok to me

for p in self.processes:
p.terminate()
p.join()
self.processes.clear()
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the latter allocates a new array object/id?
Which could potentially cause problems if other objects are holding references to self.processes specifically.

If so: that's comment-worthy, cause it's a nonlocal concern.

self.init_processes(test_queue)
results = self.collect_results()
try:
self.init_processes(test_queue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably if init-ing fails we might need to clean up... forget what it does and if/how it can fail though.



def g_testing_thread(work_queue, result_queue, temp_dir):
signal.signal(signal.SIGINT, signal.SIG_IGN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't speak signal

Base automatically changed from master to main March 8, 2021 23:49
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Apr 16, 2022
@stale stale bot closed this Jun 13, 2022
sbc100 added a commit that referenced this pull request Nov 16, 2022
Use the `Pool` seems to address the long standing issue we have
had with interrupting the test suite using `crtl-C`.

Its also a lot less code.

Fixes: #18213
Supersedes: #11299
sbc100 added a commit that referenced this pull request Nov 16, 2022
Use the `Pool` seems to address the long standing issue we have
had with interrupting the test suite using `crtl-C`.

Its also a lot less code.

Fixes: #18213
Supersedes: #11299
RReverser pushed a commit that referenced this pull request Nov 16, 2022
Use the `Pool` seems to address the long standing issue we have
had with interrupting the test suite using `crtl-C`.

Its also a lot less code.

Fixes: #18213
Supersedes: #11299
sbc100 added a commit that referenced this pull request Nov 16, 2022
Use the `Pool` seems to address the long standing issue we have
had with interrupting the test suite using `crtl-C`.

Its also a lot less code.

Fixes: #18213
Supersedes: #11299
sbc100 added a commit that referenced this pull request Nov 16, 2022
Use the `Pool` seems to address the long standing issue we have
had with interrupting the test suite using `crtl-C`.

Its also a lot less code.

Fixes: #18213
Supersedes: #11299
sbc100 added a commit that referenced this pull request Nov 16, 2022
Use the `Pool` seems to address the long standing issue we have
had with interrupting the test suite using `crtl-C`.

Its also a lot less code.

Fixes: #18213
Supersedes: #11299
sbc100 added a commit that referenced this pull request Nov 16, 2022
Use the `Pool` seems to address the long standing issue we have
had with interrupting the test suite using `crtl-C`.

Its also a lot less code.

Fixes: #18213
Supersedes: #11299
sbc100 added a commit that referenced this pull request Nov 16, 2022
Use the `Pool` seems to address the long standing issue we have
had with interrupting the test suite using `crtl-C`.

Its also a lot less code.

Fixes: #18213
Supersedes: #11299
sbc100 added a commit that referenced this pull request Nov 16, 2022
Use the `Pool` seems to address the long standing issue we have
had with interrupting the test suite using `crtl-C`.

Its also a lot less code.

Fixes: #18213
Supersedes: #11299
@sbc100 sbc100 deleted the test_runner_key_interrupt branch March 7, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants