[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #32103 [Core Tor/Tor]: Subsystem "thread_cleanup" is never called
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs