Filed https://github.com/golang/go/issues/72948,
https://github.com/golang/go/issues/72949,
and https://github.com/golang/go/issues/72950 to track the suggested
improvements.
On Wednesday, March 19, 2025 at 9:52:17 AM UTC-4 Gavra wrote:
Thanks Michael.
My opinion is
1. It should be mentioned explicitly in the doc that a blocking finalizer
will cause future objects with finalizers to leak. Probably next to the "A
single goroutine runs all finalizers for a program..." section.
Sure, I filed a bug to add a sentence to the documentation and an example
to the GC guide.
2. Go should provide a way to know about it when it happens. For example,
we could have runtime.LastFinalizer() which returns {StartTime, Trace}.
Users may choose to monitor it to detect the issue and its source.
I'm not sure it makes sense to add a new API for this. A new API is always
a complicated affair, whereas the runtime/metrics package and existing
diagnostics (like possibly an analysis in the execution trace tooling) can
support detecting common issues without having users change their code
(much).
Also, I'm personally interested in an explicit debug mode that detects
issues with finalizers and cleanups, since that can also be far more
thorough. A bespoke API would likely only be able to help with some
narrower set of issues.
3. I am not sure if it is intentional, but the goroutine dump indicates
finq only if debug=1 is used.
It's classified as a system goroutine, but it is supposed be re-classified
as a user goroutine while it is executing user code, such that it shows up
in regular stack traces. There may be a bug in this reclassification. Do
you have a reproducer? That would help isolate the issue.
4. Maybe we can add pprof/sys to indicate such metrics/potential issues?
Mentioned above, but yes. Not sure a pprof profile is a good fit, though I
suppose you could imagine a "finalizer duration" profile.
5. Regarding the single routine vs multi-routine. Yes, this is clear. Did
you consider having a finalizer routine per package? I think this is a
significant deficit since it break isolation. Either way, I find providing
1 and 2 sufficient.
Unfortunately no, this was not considered. Reference queues a la Java
probably should have been discussed more in the design for AddCleanup.
Simultaneously, given that we control goroutine scheduling in the runtime,
I do think there are ways to make this better in the future without having
each package manage its own queue.
Do you want me to start a formal discussion in golang/go or will you take
it from here?
I think the issues I filed are sufficient, but feel free to file more if
you disagree.
Thanks again.
Thank you for reporting back to this thread!
On Wednesday, 19 March 2025 at 15:24:46 UTC+2 Michael Knyszek wrote:
> Unfortunately, this is pretty much how such features tend to be designed
to keep overheads low.
Er, sorry, this is actually a little more nuanced. Creating a new goroutine
for each finalizer/cleanup could cause a different problem of generating
too many goroutines (so, too much memory being used) in addition to the
cost of creating and scheduling new goroutines, and once that behavior is
relied on it's very difficult to change. For example, context.AfterFunc has
this behavior, and that's an example where the new goroutine overhead has
been reported to us as causing performance issues as well. I suspect there
just isn't going to be one right answer for all cases. Cleanups and
finalizers are relatively low level, so the scales tilted more toward
having users do their own scheduling, at the risk of the blocking issues
you ran into. (As well as the fact that cleanups and finalizers have more
loose guarantees in general.)
On Wednesday, March 19, 2025 at 9:05:30 AM UTC-4 Michael Knyszek wrote:
Sorry you ran into this issue.
Unfortunately, this is pretty much how such features tend to be designed to
keep overheads low. You make a good point that we should have more
diagnostics to detect when this happens. We can add a `runtime/metrics`
metric, or perhaps add extend the capabilities of an experimental debug
mode I'm working on for cleanups and finalizers (https://go.dev/cl/634599).
At the very least, I'll make note of this in https://go.dev/doc/gc-guide.
Note also that even though `runtime.AddCleanup` is documented as running
cleanups concurrently,
1. In Go 1.24 we don't actually do that, so it won't help you to switch in
the short term. I have a patch for Go 1.25 to do that properly. (
https://go.dev/cl/650697)
2. Even once we can run cleanups concurrently, it's still hazardous to
spend a long time in a cleanup. The Go 1.25 implementation will not run
each cleanup in its own goroutine, as that is just too expensive and very
hard to change. You can still exhaust the finite number of goroutines used
to run cleanups.
Java Cleaners have a similar limitation in that each Cleaner runs all its
actions on a single thread. ("The execution of the cleaning action is
performed by a thread associated with the cleaner. [...] The thread runs
until all registered cleaning actions have completed and the cleaner itself
is reclaimed by the garbage collector.", from
https://docs.oracle.com/javase/9/docs/api/java/lang/ref/Cleaner.html) There
is a difference here in that you can create as many cleaners as you want,
AFAICT.
I'll note that we might be able to do something more complicated in the
future in which we "detach" a cleanup goroutine if we detect that it blocks
or runs too long at runtime. But just to set expectations, I don't think
there's a high likelihood that we'll do something like this any time soon.
On Wednesday, March 19, 2025 at 8:49:59 AM UTC-4 Gavra wrote:
Yes, even if it was only a few ms, they should have run it in a goroutine.
On Wednesday, 19 March 2025 at 14:17:56 UTC+2 Robert Engels wrote:
The docs:
“ A single goroutine runs all finalizers for a program, sequentially. If a
finalizer must run for a long time, it should do so by starting a new
goroutine.”
So the code did not follow the api docs - as apparently the close() is
indefinite.
On Mar 19, 2025, at 6:46 AM, Gavra <gav...@gmail.com> wrote:
Yes, I intend to open a bug for them.
I agree that one should not relay on the execution of finalizers. But, the
fact that the runtime just piles them up because one package did wrong is
outrageous.
By the way, I am not sure why but I can see the mfinal routine only when
running my program from Goland; When I build it manually the routine is
invisible using pprof.
On Wednesday, 19 March 2025 at 13:38:04 UTC+2 Robert Engels wrote:
In fact the code you reference - the close() - does things the Go docs warn
specifically not to do.
You may be better off using runtime.AddCleanup()
On Mar 19, 2025, at 6:32 AM, Robert Engels <ren...@ix.netcom.com> wrote:
In principle, I would argue that there is a correctness problem. You should
not rely on finalizers ever - they are catches and often optional - so the
design relying on finalizers to run is what is broken.
In the real world they can make solving certain problems much easier -
especially with shared resources. I know Java has deprecated them in lieu
of other technologies like Cleaners.
On Mar 19, 2025, at 6:19 AM, Gavra <gav...@gmail.com> wrote:
https://github.com/hirochachacha/go-smb2/blob/c8e61c7a5fa7bcd1143359f071f9425a9f4dda3f/client.go#L1063
We are looking for why exactly it blocked (probably incorrect ctx) but I
think this close should run in a goroutine.
On Wednesday, 19 March 2025 at 12:29:34 UTC+2 Brian Candler wrote:
On Wednesday, 19 March 2025 at 09:55:58 UTC Gavra wrote:
This finalizer blocks the runtime's finalizer goroutine
Out of interest, what made it block? Was the finalizer doing some channel
communication, for example?
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to golang-nuts...@googlegroups.com.
To view this discussion visit
https://groups.google.com/d/msgid/golang-nuts/5c7c034d-3bc6-4fe5-82bb-a318310bd82fn%40googlegroups.com
<https://groups.google.com/d/msgid/golang-nuts/5c7c034d-3bc6-4fe5-82bb-a318310bd82fn%40googlegroups.com?utm_medium=email&utm_source=footer>
.
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to golang-nuts...@googlegroups.com.
To view this discussion visit
https://groups.google.com/d/msgid/golang-nuts/6AABBB52-5BC6-4874-900B-C423E37952C4%40ix.netcom.com
<https://groups.google.com/d/msgid/golang-nuts/6AABBB52-5BC6-4874-900B-C423E37952C4%40ix.netcom.com?utm_medium=email&utm_source=footer>
.
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to golang-nuts...@googlegroups.com.
To view this discussion visit
https://groups.google.com/d/msgid/golang-nuts/75da8ea2-c099-483c-969a-ab99a91da2a4n%40googlegroups.com
<https://groups.google.com/d/msgid/golang-nuts/75da8ea2-c099-483c-969a-ab99a91da2a4n%40googlegroups.com?utm_medium=email&utm_source=footer>
.
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
To view this discussion visit
https://groups.google.com/d/msgid/golang-nuts/d7ff0b24-91f1-4bfa-84c7-3ccd99901ba3n%40googlegroups.com.
