[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:
--------------------------------------+------------------------------------
Changes (by catalyst):

 * reviewer:  catalyst => nickm


Comment:

 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.

 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()`?

 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.

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

 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?

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

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