sbws AsyncResults have no timeout

After sbws queues an AsyncResult, it will wait forever for the result to be ready: https://github.com/torproject/sbws/blob/ee64d76df54ceb3a3c9e1e2a797fd70d68bb0035/sbws/core/scanner.py#L359-L364

If at least one result hangs, then sbws will hang, because AsyncResult.ready() does not have a timeout.

Instead, sbws should call AsyncResult.wait([timeout]) on each result, after calling pool.apply_async() on a large number of results.

See https://docs.python.org/3/library/multiprocessing.html#multiprocessing.pool.AsyncResult.wait

changed milestone to %sbws: 1.1.x-final

Trac:
Parent Ticket: #28663 (moved)

added component::core tor/sbws milestone::sbws: 1.1.x-final owner::juga parent::28663 points::1 priority::medium resolution::fixed reviewer::nickm severity::normal status::closed type::defect version::sbws 1.0.2 labels

This ticket probably needs to be done at the same time as #28865 (moved).

Replying to teor:

After sbws queues an AsyncResult, it will wait forever for the result to be ready: https://github.com/torproject/sbws/blob/ee64d76df54ceb3a3c9e1e2a797fd70d68bb0035/sbws/core/scanner.py#L359-L364

If at least one result hangs, then sbws will hang, because AsyncResult.ready() does not have a timeout.

In theory this won't happen, since both circuits and requests have a timeout. Also the sleep happen in the main thread, not the thread that is getting the results

Instead, sbws should call AsyncResult.wait([timeout]) on each result, after calling pool.apply_async() on a large number of results.

See https://docs.python.org/3/library/multiprocessing.html#multiprocessing.pool.AsyncResult.wait

I'll try this anyway

I looked more at this. wait is the method of an event (https://github.com/python/cpython/blob/master/Lib/multiprocessing/pool.py#L662), and it blocks (https://docs.python.org/3/library/threading.html?highlight=threading#threading.Event.wait) sleep is not blocking because happen in the main thread. Probably the cause (or at least the main one) is #28897 (moved).

Replying to juga:

I looked more at this. wait is the method of an event (https://github.com/python/cpython/blob/master/Lib/multiprocessing/pool.py#L662), and it blocks (https://docs.python.org/3/library/threading.html?highlight=threading#threading.Event.wait)

a maximum of timeout. get with timeout uses wait, and that's what we want since pending results that didn't trigger either callback or callback error have not been gotten, and get give us either the exception or the value.

After merging #28932 (moved), this is an improvement, can wait to 1.1 milestone.

https://github.com/torproject/sbws/pull/341

Trac:
Status: new to needs_review
Milestone: sbws: 1.0.x-final to sbws: 1.1.x-final
Cc: juga, teor to N/A

reassigning so that the ticket has owner

Trac:
Status: needs_review to assigned
Owner: N/A to juga

Trac:
Status: assigned to needs_review

Trac:
Points: N/A to 1

Because of #28865 (moved), this needs to be re-thought, to don't make it yet more complicated.

Trac:
Status: needs_review to needs_revision

Solving also #28865 (moved): https://github.com/torproject/sbws/pull/342 Moving to 1.1 milestone since #28932 (moved) check progress and logs error.

Trac:
Status: needs_revision to needs_review

Trac:
Reviewer: N/A to nickm

This code looks reasonable to me. If you have tested it and works for you, then I'd say "merge it!" :)

For my own curiosity: why is the time cutoff set to "the number of releys to measure multiplied by TIMEOUT_MEASUREMENTS (around 90mins)"?

Trac:
Status: needs_review to merge_ready

Replying to nickm:

For my own curiosity: why is the time cutoff set to "the number of releys to measure multiplied by TIMEOUT_MEASUREMENTS (around 90mins)"?

Well spotted. Actually when measuring 300 relays it takes around 90min. In the case a relay doesn't get measured in 3min, there's no need to wait that 300 relays take each of them 3min, it can just finish when there hasn not been any relay measured in that time. I commited a fixup to do that, tested the whole loop in the public network and works.

Merged.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

changed time estimate to 8h

mentioned in issue #28865 (moved)

moved to tpo/network-health/sbws#28864 (closed)

mentioned in issue tpo/network-health/sbws#28865 (closed)