Opened 8 months ago

Closed 7 months ago

#28869 closed defect (fixed)

KeyboardInterrupt will cause a callback error and does not close the thread pool cleanly

Reported by: juga Owned by:
Priority: Medium Milestone: sbws: 1.0.x-final
Component: Core Tor/sbws Version: sbws: 1.0.2
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: #28663 Points:
Reviewer: asn Sponsor:

Description

KeyboardInterrupt is being catched in main (https://github.com/torproject/sbws/blob/ee64d76df54ceb3a3c9e1e2a797fd70d68bb0035/sbws/core/scanner.py#L394), it's not possible from there to call pool.close() so that the pool terminates the threads.
What is more, it'll result in a callback error to the pool.
When the operator wants to cancel sbws, it should close all the threads, don't call callback error, and don't show all the exceptions that the threads hit not being able to finish.

Child Tickets

Change History (8)

comment:1 Changed 8 months ago by juga

Component: - Select a componentCore Tor/sbws

comment:2 Changed 8 months ago by juga

Status: newneeds_review

comment:3 Changed 8 months ago by juga

when #28897 is merged, callback error will stop receiving TypeError
when #28870 is merged, callback error will stop receiving AssertionError

comment:4 Changed 7 months ago by asn

Reviewer: asn

comment:5 Changed 7 months ago by asn

Status: needs_reviewneeds_revision

Did a review, but it's mostly questions since I'm not that familiar with the codebase and this was a bigger patch set than I thought! Feel free to correct me wherever I'm blatantly wrong.

comment:6 in reply to:  5 Changed 7 months ago by juga

Status: needs_revisionneeds_review

Replying to asn:

Did a review, but it's mostly questions since I'm not that familiar with the codebase and this was a bigger patch set than I thought! Feel free to correct me wherever I'm blatantly wrong.

The important changes are:

  • move SIGINT before running the main loop
  • Check end event before starting main loop and before starting new measurments, to don't have to wait for them to finish
  • Catch some exceptions earlier, so see them when they happen (and create a ResultError with them) so that they are not triggered by the callback error when finishing.
  • Stop the threads when SIGINT or SIGTERM are triggered.

I replied to the questions.

comment:7 Changed 7 months ago by asn

Status: needs_reviewmerge_ready

LGTM!

comment:8 Changed 7 months ago by juga

Resolution: fixed
Status: merge_readyclosed

PR squashed and merged.

Note: See TracTickets for help on using tickets.