Would not Stopping time.Ticker leak memory?

[Fumin Wang]

Hi

Although, not explicitly stated in http://golang.org/pkg/time/#Ticker , would not calling time.(*Ticker).Stop() leak memory?

The reason why I suspect there'd be a leak is because while tracing code at http://golang.org/src/pkg/runtime/time.goc#L41 , I noticed that a Ticker actually occupies memory in a global heap "timers".

I wonder is this true, or is there some misunderstanding on my part about Ticker's implementation?

If it's indeed necessary for users of time.Ticker to Stop their tickers, would it be better to enhance the documentation http://golang.org/pkg/time/#Ticker to warn of this fact?

[mikespook]

I don't think it's a memory leak.

* If a ticker is going to be stop, the Stop method always should be called. If we won't, the ticker won't stop. It will be still working right?

* If a ticker is still working, why do we think about that's a memory leak?

Another example is about the network connection. If you never closed a connection until the application exited. Can we say the connection has a memory leak? (Assume the OS can handle and release all resources the application used)

Anyway, I really don't know if it is better to make comments in the doc.

--
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.
For more options, visit https://groups.google.com/d/optout.

--
Xing Xing (邢星)
mikespook <mike...@gmail.com>
http://mikespook.com

[Rui Ueyama]

I think it's arguable whether or not it's a programmer's responsibility to explicitly stop a Ticker.

There are some types that needs an explicit resource management, such as file, which you have to close after use. Usually such types are representing external resources. OTOH you don't have to close a channel made by make(chan ...). It will be gc'ed if it's not reachable. I'd think I prefer not to stop a Ticker after use.

[Fumin Wang]

Hi

Thanks for your input, so it seems that it's true that

'If a ticker is going to be stop, the Stop method always should be called'

quoting Mikespook.

In my opinion, this is something that dearly needs to be included in the docs http://golang.org/pkg/time/#Ticker much like how it is emphasized to Close() after reading from resp.Body http://golang.org/pkg/net/http/#Client.Get . 

On the other hand, I agree with Ueyama that the preferable behaviour is for the runtime to Stop the Ticker when it is garbage collected. I wonder is it the case that this presents extreme technical difficulty so that Ticker is not implemented as such?

[James Bardin]

The timer has been designed so that without any references it can be garbage collected, once it's been fired.

This means it either has to wait until the interval triggers a send on its internal channel, or you need to call Stop. 

[Fumin Wang]

Hi James

That is good news, are you able to spare time to point out where in the source code or documentation could I confirm this?

I admit I'm not a seasoned C programmer, but after studying through runtime/time.goc, time/sleep.go, and time/tick.go, I still couldn't convince myself that the garbage collector could garbage collect a Ticker when it is no longer referenced. Thanks in advance.

[Ian Lance Taylor]

On Wed, May 14, 2014 at 6:30 AM, James Bardin <j.ba...@gmail.com> wrote:
>
> The timer has been designed so that without any references it can be garbage
> collected, once it's been fired.
> This means it either has to wait until the interval triggers a send on its
> internal channel, or you need to call Stop.

I don't think that's true for a time.Ticker. There will be a
reference from the timers variable in runtime/time.goc, so the ticker
will never be garbage collected.

If the Ticker can be garbage collected, then we have a different
problem: a dangling pointer in timers.

Ian

[Ian Lance Taylor]

I think we should just figure out how to fix the problem. It ought to
be possible to fix it using a finalizer.

Ian

[James Bardin]

On Wed, May 14, 2014 at 10:12 AM, Ian Lance Taylor <ia...@golang.org> wrote:

On Wed, May 14, 2014 at 6:30 AM, James Bardin <j.ba...@gmail.com> wrote:
>
> The timer has been designed so that without any references it can be garbage
> collected, once it's been fired.
> This means it either has to wait until the interval triggers a send on its
> internal channel, or you need to call Stop.

I don't think that's true for a time.Ticker.  There will be a
reference from the timers variable in runtime/time.goc, so the ticker
will never be garbage collected.

If the Ticker can be garbage collected, then we have a different
problem: a dangling pointer in timers.

Oh true, a Ticker needs to be stopped; for some reason I was only thinking about time.Timer. 

[mikespook]

According to the implementation of `timerproc` function[1].

If the t->period upper than zero, then keep it in the heap, otherwise remove the runtimeTimer instance from heap.

The mainly difference between time.Ticker and time.Timer is the `period` filed of runtimeTimer[2].

[1]. http://golang.org/src/pkg/runtime/time.goc#L203

[2]. http://golang.org/src/pkg/time/sleep.go#L18

[Dmitry Vyukov]

It would require some kind of weak references (at least in runtime)
which we don't have yet.
Also it will significantly penalize programs that do call Stop, as
they will need to set/reset the finalizer.
That's not to say that it would not be nice to fix it.

[Fumin Wang]

I have the impression that channels themselves rely on the same timers structure, too. If this is true (please forgive and do correct me if I'm wrong), are channels garbage collected also via finalizers, or if not, is it possible for Tickers to leverage the same mechanism as do channels?

In the meantime, does it make sense to improve http://golang.org/pkg/time/#Ticker on the necessacity to call Stop()? If so, I'd more than glad to contribute and help. 

[Dmitry Vyukov]

I have an idea of making time channels (timer, ticker) "active".
Now we have "sync" and "async" channel implementation (and my
lock-free channels proposal also introduces "empty" type), "active"
time channels will be a yet another channel type. Such channels will
store delay, period, last send time, etc. A goroutine which tries to
receive from such a channel will directly update the fields, or block
until the next tick.
This will eliminate one "hop" from all timer operations, remove the
necessity to register/unregister timers with runtime, and as a
consequence make Stop no-op.

[Dmitry Vyukov]

On Wed, May 14, 2014 at 7:21 PM, Dmitry Vyukov <dvy...@google.com> wrote:
> On Wed, May 14, 2014 at 6:43 PM, Dmitry Vyukov <dvy...@google.com> wrote:
>> On Wed, May 14, 2014 at 6:15 PM, Ian Lance Taylor <ia...@golang.org> wrote:
>>> On Tue, May 13, 2014 at 9:16 PM, Fumin Wang <awaw...@gmail.com> wrote:
>>>>
>>>> Although, not explicitly stated in http://golang.org/pkg/time/#Ticker ,
>>>> would not calling time.(*Ticker).Stop() leak memory?
>>>> The reason why I suspect there'd be a leak is because while tracing code at
>>>> http://golang.org/src/pkg/runtime/time.goc#L41 , I noticed that a Ticker
>>>> actually occupies memory in a global heap "timers".
>>>> I wonder is this true, or is there some misunderstanding on my part about
>>>> Ticker's implementation?
>>>> If it's indeed necessary for users of time.Ticker to Stop their tickers,
>>>> would it be better to enhance the documentation
>>>> http://golang.org/pkg/time/#Ticker to warn of this fact?
>>>
>>> I think we should just figure out how to fix the problem. It ought to
>>> be possible to fix it using a finalizer.
>>
>>
>> It would require some kind of weak references (at least in runtime)
>> which we don't have yet.
>> Also it will significantly penalize programs that do call Stop, as
>> they will need to set/reset the finalizer.
>> That's not to say that it would not be nice to fix it.

+golang-dev

[Ian Lance Taylor]

On Wed, May 14, 2014 at 8:14 AM, Fumin Wang <awaw...@gmail.com> wrote:
>
> I have the impression that channels themselves rely on the same timers
> structure, too. If this is true (please forgive and do correct me if I'm
> wrong), are channels garbage collected also via finalizers, or if not, is it
> possible for Tickers to leverage the same mechanism as do channels?

Channels do not use the timers structure. There is no need for them
to do so, as channels do not time out.

> In the meantime, does it make sense to improve
> http://golang.org/pkg/time/#Ticker on the necessacity to call Stop()? If so,
> I'd more than glad to contribute and help.

Thanks, but if we can in fact fix this in 1.4, then I would prefer to
not modify the documentation now.

Ian

[Ian Lance Taylor]

But what would help is to open an issue (http://golang.org/issue/new)
so that we remember the problem.

Ian

[Rui Ueyama]

I have an idea that if a goroutine is able to know in some way that all receive or bi-directional ends of a send channel have already been garbage collected, it can stop sending to the channel and kill itself. Because nobody is listening the channel, the goroutine usually want to stop what it is doing. It could be like handling SIGPIPE for goroutines.

I'd think it's also useful to prevent goroutine leaking.

Ian

[Ian Lance Taylor]

On Wed, May 14, 2014 at 10:08 AM, Rui Ueyama <ru...@google.com> wrote:
>
> I have an idea that if a goroutine is able to know in some way that all
> receive or bi-directional ends of a send channel have already been garbage
> collected, it can stop sending to the channel and kill itself. Because
> nobody is listening the channel, the goroutine usually want to stop what it
> is doing. It could be like handling SIGPIPE for goroutines.
>
> I'd think it's also useful to prevent goroutine leaking.

We've historically resisted doing that because it can cause large
parts of your program to evaporate with no trace, leaving you with an
empty stack dump and no explanation for where all your goroutines
went.

But perhaps we could capture a stack dump as they go so that we have
something to print if the program crashes.

Ian

[Dmitry Vyukov]

Full stack dumps will still leak memory, as they will be unique per goroutine.
If we capture only PCs and dedup them, then it may work.

[Rui Ueyama]

I was not thinking that a goroutine would be killed unconditionally. What I was thinking instead was to provide some mean, such as a function or something, to ask if a send channel has receive ends. Users use it to do whatever it wants. It may want to terminate a goroutine, but it's up to the user.

2014/05/14 10:43 "Ian Lance Taylor" <ia...@golang.org>:

[Kyle Lemons]

On Wed, May 14, 2014 at 11:10 AM, 'Rui Ueyama' via golang-nuts <golan...@googlegroups.com> wrote:

I was not thinking that a goroutine would be killed unconditionally. What I was thinking instead was to provide some mean, such as a function or something, to ask if a send channel has receive ends. Users use it to do whatever it wants. It may want to terminate a goroutine, but it's up to the user.

You can already effectively ask whether sending a value on a channel would block.  This is probably about as close as we want to get to what you're suggesting; asking whether there is a goroutine in existence which could receive a value amounts to the halting problem.

2014/05/14 10:43 "Ian Lance Taylor" <ia...@golang.org>:

On Wed, May 14, 2014 at 10:08 AM, Rui Ueyama <ru...@google.com> wrote:
>
> I have an idea that if a goroutine is able to know in some way that all
> receive or bi-directional ends of a send channel have already been garbage
> collected, it can stop sending to the channel and kill itself. Because
> nobody is listening the channel, the goroutine usually want to stop what it
> is doing. It could be like handling SIGPIPE for goroutines.
>
> I'd think it's also useful to prevent goroutine leaking.

We've historically resisted doing that because it can cause large
parts of your program to evaporate with no trace, leaving you with an
empty stack dump and no explanation for where all your goroutines
went.

But perhaps we could capture a stack dump as they go so that we have
something to print if the program crashes.

Ian

--

[Rui Ueyama]

No, it's different from the halting problem.

What I wrote is, if no one has references to ch or recv in the following code, both will be garbage collected, and send will become a send channel that has no reading ends. That fact can be detected by the system, and we can tell the user the fact in some way.

ch := make(chan int)

send := <-chan int(ch)
recv := (chan<- int)(ch)

[Ian Lance Taylor]

On Wed, May 14, 2014 at 12:50 PM, Rui Ueyama <ru...@google.com> wrote:
>
> What I wrote is, if no one has references to ch or recv in the following
> code, both will be garbage collected, and send will become a send channel
> that has no reading ends. That fact can be detected by the system, and we
> can tell the user the fact in some way.
>
> ch := make(chan int)
> send := <-chan int(ch)
> recv := (chan<- int)(ch)

It's an interesting idea. It's complicated by the fact that in the
current system send and recv have the same value, they just have
different types. This can be seen by, e.g., using
reflect.ValueOf(x).Pointer() on both send and recv; you will get the
same value.

Ian

[Rui Ueyama]

Is that a part of the language spec, or just that the current implementations do that that way?

By the way, obviously, we could also provide the inverse; if references to ch or send are lost, recv will become a receiving channel that has no sending ends, and we can let the user know about that, too.

[Ian Lance Taylor]

On Wed, May 14, 2014 at 1:19 PM, Rui Ueyama <ru...@google.com> wrote:
> On Wed, May 14, 2014 at 1:07 PM, Ian Lance Taylor <ia...@golang.org> wrote:
>>
>> On Wed, May 14, 2014 at 12:50 PM, Rui Ueyama <ru...@google.com> wrote:
>> >
>> > What I wrote is, if no one has references to ch or recv in the following
>> > code, both will be garbage collected, and send will become a send
>> > channel
>> > that has no reading ends. That fact can be detected by the system, and
>> > we
>> > can tell the user the fact in some way.
>> >
>> > ch := make(chan int)
>> > send := <-chan int(ch)
>> > recv := (chan<- int)(ch)
>>
>> It's an interesting idea. It's complicated by the fact that in the
>> current system send and recv have the same value, they just have
>> different types. This can be seen by, e.g., using
>> reflect.ValueOf(x).Pointer() on both send and recv; you will get the
>> same value.
>
>
> Is that a part of the language spec, or just that the current
> implementations do that that way?

The reflect package is not part of the language spec. However, it is
covered by the Go 1 guarantee. The question is whether that guarantee
requires that the Pointer method return the same value for a channel
converted to send-only or recv-only. I'm not sure what the answer
should be for that.

Anyhow I'm not saying this makes the idea impossible, it's just a
complication.

Ian

[Rui Ueyama]

Yeah, I could see it complicates that.

I feel like this idea could possibly greatly simplify the leaking goroutine issue in many cases. It might be useful in other cases too. It might worth pursuing a bit more.

[Rui Ueyama]

Thinking about this a bit deeply and came up with a different idea.

Assume that the runtime can detect that a receiving channel R has no sending ends. If a goroutine G blocks on R, we can prove that G will never ever wake up, because there's no way to send a message to R to wake G up.

In that case, we can garbage collect goroutine G without changing the meaning of the program, can't we?

[Ian Lance Taylor]

On Wed, May 14, 2014 at 3:35 PM, Rui Ueyama <ru...@google.com> wrote:
>
> Thinking about this a bit deeply and came up with a different idea.
>
> Assume that the runtime can detect that a receiving channel R has no sending
> ends. If a goroutine G blocks on R, we can prove that G will never ever wake
> up, because there's no way to send a message to R to wake G up.
>
> In that case, we can garbage collect goroutine G without changing the
> meaning of the program, can't we?

Without changing the meaning of the program, sure. But not without
changing the programmer's ability to debug the program. A programmer
may expect that some goroutine has quietly exited. It can be useful
to know that it is instead sitting around waiting for input that it
will never receive.

Ian

[Matt Harden]

It would be nice to automatically place any channel that has either no possible senders or no possible receivers into a "broken" state. Then perhaps a runtime function could be provided that creates a special read-only channel that would close whenever the original channel is broken. For example:

select {

case x <- c:

    fmt.Printf("%v was read from the channel.\n", x)

case _ <- chan.broken(c):

    fmt.Println("No possible senders on this channel!")

}

The same would work for sending:

select {

case c <- x:

    fmt.Printf("%v was sent on the channel.\n", x)

case _ <- chan.broken(c):

    fmt.Println("No possible receivers on this channel!")

}

I don't know how hard it would be to implement, but at least I believe it wouldn't break the Go 1 compatibility promise. There are possible variations on this. Maybe a runtime function that takes a channel and returns a modified version of it that treats a broken channel as if it had been closed; thus panicking on write and returning a zero value on read.

Regards,

Matt

[Kyle Lemons]

On Wed, May 14, 2014 at 6:02 PM, Matt Harden <matt....@gmail.com> wrote:

It would be nice to automatically place any channel that has either no possible senders or no possible receivers into a "broken" state. Then perhaps a runtime function could be provided that creates a special read-only channel that would close whenever the original channel is broken. For example:

select {

case x <- c:

    fmt.Printf("%v was read from the channel.\n", x)

case _ <- chan.broken(c):

    fmt.Println("No possible senders on this channel!")

}

The same would work for sending:

select {

case c <- x:

    fmt.Printf("%v was sent on the channel.\n", x)

case _ <- chan.broken(c):

    fmt.Println("No possible receivers on this channel!")

}

I don't know how hard it would be to implement, but at least I believe it wouldn't break the Go 1 compatibility promise. There are possible variations on this. Maybe a runtime function that takes a channel and returns a modified version of it that treats a broken channel as if it had been closed; thus panicking on write and returning a zero value on read.

What's your goal?  I assert that you can't garbage collect the goroutines without possibly changing the behavior of the program because of finalizers, and that even if you do collect its memory you have to keep its stack trace around, so while it might reduce the magnitude your memory leaks, it won't get rid of them.  Giving programmers the tools to check these sorts of states at runtime seems like you're asking them to program defensively and waste effort on something that looks like it would start to feel like malloc/free bookkeeping.  Programs like this are bad programs, I'm not sure we should spend a lot of time on figuring out how to make them slightly better, especially if it compromises our ability to debug them.

[Sokolov Yura]

Why "broken"? Why not just close the channel?

[Fumin Wang]

Thanks all, I have filed a ticket https://code.google.com/p/go/issues/detail?id=8001 describing this issue. Hopefully, we could come up with a satisfactory solution in 1.4.

In the meantime, I'll be guarding myself with the follow practice:

ticker := time.NewTicker(duration)

defer ticker.Stop()

On Thu, May 15, 2014 at 3:03 PM, Sokolov Yura <funny....@gmail.com> wrote:

Why "broken"? Why not just close the channel?

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/Chx1tCs2QGg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.

[John Waycott]

Would it also be complicated if the channel is blocked in a select? A lone sender goroutine for example could wake up by other means and "create" the receiving goroutine.

In any event,  I ran into this issue recently by creating hundreds of timers without stopping the timer when its goroutine quit. It ended up actually consuming CPU.

[Rui Ueyama]

On Thu, May 15, 2014 at 6:17 AM, John Waycott <java...@cox.net> wrote:

Would it also be complicated if the channel is blocked in a select? A lone sender goroutine for example could wake up by other means and "create" the receiving goroutine.

Once you only have a channel C of type <-chan X, you cannot make another channel of type chan<- X for C (unless you use reflect package, as Ian pointed out). You can make a different channel instance, but a value sent to it won't appear at C. It appears at a different channel.

By the way I think my concern on this idea is that the existence of GC will change the meaning of the program. GC is usually totally invisible -- it silently reclaim unreferenced values, and whether it exists or not doesn't change the behavior of the program. And that's a good thing. So, if a channel changes its behavior depending on GC's presence, I feel it's a bit weird -- or wrong.

But there are some exceptions there, such as weak references or finalizers. It may be able to be considered as a "weak channel", that returns an error if the other ends are all reclaimed. Well, that's just an idea...

[Patrick Mézard]

https://code.google.com/p/go/source/detail?r=7e130e3f9a4f

--
Patrick

[Юрий Соколов]

15.05.2014 21:33 пользователь "'Rui Ueyama' via golang-nuts" <golan...@googlegroups.com> написал:

>
> By the way I think my concern on this idea is that the existence of GC will change the meaning of the program. GC is usually totally invisible -- it silently reclaim unreferenced values, and whether it exists or not doesn't change the behavior of the program. And that's a good thing. So, if a channel changes its behavior depending on GC's presence, I feel it's a bit weird -- or wrong.

But I feel it right: I want my code to know is there someone who waits for results (or will give some)?

>
> But there are some exceptions there, such as weak references or finalizers. It may be able to be considered as a "weak channel", that returns an error if the other ends are all reclaimed. Well, that's just an idea...

Doesn't "closed channel" exactly matches your "weak channel"?