[tor-bugs] #28866 [Core Tor/sbws]: ResultDump.queue.put() can hang if the queue is full

[Tor Bug Tracker & Wiki]

#28866: ResultDump.queue.put() can hang if the queue is full
---------------------------+-----------------------------------
 Reporter:  teor           |          Owner:  juga
     Type:  defect         |         Status:  needs_revision
 Priority:  Medium         |      Milestone:  sbws: 1.0.x-final
Component:  Core Tor/sbws  |        Version:  sbws: 1.0.2
 Severity:  Normal         |     Resolution:
 Keywords:                 |  Actual Points:
Parent ID:  #28663         |         Points:  1
 Reviewer:  mikeperry      |        Sponsor:
---------------------------+-----------------------------------

Comment (by juga):

 Replying to [comment:7 mikeperry]:
 > Hrmm. I might need to look at a lot more of the sbws code, but I don't
 think the patch is the best way to address the possibility that the queue
 may be full. If the queue can also be added from another thread, then
 there still can be a race where it ends up full after the loop check but
 before the next statement.

 I've never seen the case where the queue is full and it's very unlikely
 that it'll get full by other thread, since measurement threads take more
 than 5 seconds to complete, and the the `ResultDump` thread is getting
 measurements from the queue every second
 (https://github.com/torproject/sbws/blob/ada70d4f85ac7ce6f9847eeae832ec7620106470/sbws/lib/resultdump.py#L589)

 > I would instead make it do something like:
 >
 > {{{
 > while True:
 >   try:
 >     result_dump.queue.put(measurement_result, timeout=1)
 >   except Full as e:
 >     log.info('The results queue is full, after 1 second it should not be
 full')
 > }}}
 >
 > That way, if the timeout hits because the queue is full for a second,
 the exception is thrown, and then you log, and then you try again for
 another second (at which point it should succeed, but in case it does not,
 you just log and try again rather than raise an uncaught Full exception).

 That was my first though, but the `while True` will make this call to
 block forever, and the callback has to return immediately. From
 https://docs.python.org/3.5/library/multiprocessing.html#multiprocessing.pool.Pool.apply_async:

 "Callbacks should complete immediately since otherwise the thread which
 handles the results will get blocked."

 So something like that will be only possible if there was yet another
 thread handling the callback to don't block the main process (like
 `ResultDump` does), but that would add unneeded complexity.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28866#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online