[tor-bugs] #32103 [Core Tor/Tor]: Subsystem "thread_cleanup" is never called

[Tor Bug Tracker & Wiki]

#32103: Subsystem "thread_cleanup" is never called
--------------------------------------+------------------------------------
 Reporter:  opara                     |          Owner:  (none)
     Type:  defect                    |         Status:  needs_review
 Priority:  Medium                    |      Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor              |        Version:
 Severity:  Normal                    |     Resolution:
 Keywords:  042-should, extra-review  |  Actual Points:
Parent ID:                            |         Points:
 Reviewer:  nickm                     |        Sponsor:
--------------------------------------+------------------------------------

Comment (by opara):

 Replying to [comment:6 catalyst]:

 > Thanks for the patches!
 >
 > Overall, it looks like a good start.  There are a few changes that I
 think should happen before merging.  I'm also asking nickm to do some
 extra review in case there are subtle things related to how we use
 threads.

 Thanks for taking a look at it! I'll try to explain some of my design
 reasoning below. Let me know what changes you'd like me to make, and I can
 do that.

 ----

 > The new wrapper in tor_threads.c apparently changes the contract of
 `spawn_func()`, which says func should not return, but rather should call
 `spawn_exit()`.  The documentation of `spawn_func()` isn't changed
 accordingly.  Maybe the wrapper for the thread-local shutdown should go in
 `spawn_exit()`, instead of expecting the thread-start function to return?
 It seems like this would have been caught by a unit test that tests the
 new functionality.  (I'm not quite sure how difficult such a test would be
 to write.  Please let me know if it would be exceptionally difficult.)
 >
 > At an architectural level, I would prefer to avoid adding a new general
 interface that only has one user.  It seems to add considerable
 complexity: new fields and parameters in several places, new files, etc.
 >
 > Why not call `subsystems_thread_init()` from `spawn_func()` and
 `subsystems_thread_cleanup()` from `spawn_exit()`?

 My reason for this was to make the `start_tor_thread` functionality be an
 app-level abstraction on top of the original tor threading library.
 Calling the subsystem manager code (such as `subsystems_thread_init()`)
 from the threading library (such as within `spawn_func()`) causes the
 subsystem module to become a dependency of the threading library. My goal
 was to not have this library (in the `lib/` directory) depend on
 application-specific code (in the `app/` directory). This approach also
 means that someone using a thread (created with `start_tor_thread()`) does
 not need to worry about running `spawn_exit()` or any additional cleanup-
 code since it will happen automatically, hopefully reducing the
 possibility of bugs. For example, the threadpool already forgets to call
 `spawn_exit()` (see https://github.com/torproject/tor/pull/1412/), which
 means there would be a bug.

 Additionally, in the future someone may wish to create a new thread which
 does something very simple, and may not want that thread interacting with
 the tor subsystem manager. Integrating the subsystem manager into the
 threading library would make this impossible.

 ----

 > Minor: The patches expand `tor_run_main()` beyond the practracker limit
 of 100 lines.  (This is causing CI to fail.)  We can consider refactoring,
 or add a new exception for that function.

 Yes, I didn't want to refactor tor's main method in this PR since that
 could probably be a whole new PR on its own, but also didn't want to
 reduce the readability of this PR to satisfy the CI system. (Aside:
 practracker seems to include empty lines in the limit, but it would be
 nice if it ignored them since empty lines can make the code more
 readable). Let me know what approach you would prefer and I can make that
 change.

 ----

 > Minor: The loop direction fixup should probably be squashed before
 merge.

 I'm not sure how GitHub handles `push --force` when people have left
 comments on commits (I don't want to overwrite teor's comment), so I
 haven't done this yet. Please let me know if you'd like me to squash now
 or later.

 ----

 > Minor: There should probably be a changes file (see
 doc/HACKING/CodingStandards.md).  We can help with this if you like.
 >
 > You noticed that the crypto subsystem uses a singleton pattern
 currently.  Your patches don't seem to modify this.  Is that intentional?

 I didn't modify that in this PR since I didn't want to make too many
 changes at once (although I have modified it in my own personal changes to
 tor).

 ----

 > How will these changes interact with building tor as a library that
 possibly gets started and stopped, or loaded and unloaded?

 I'm not sure, I have no experience building tor as a library. Currently
 threads are only created when tor is running as a relay (in server mode).
 Does anyone use tor as a library when running a relay? I feel like this
 would already break things, such as the threadpool which is never freed,
 and whose work entries are also not freed when shutting down.

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