search

fyne-io / fyne

Cross platform GUI toolkit in Go inspired by Material Design
https://fyne.io/
Other
25.25k stars 1.4k forks source link

Recover panic "send on closed channel" #5214

Closed xaionaro closed 1 month ago

xaionaro commented 1 month ago

Description:

In my application I have a lot of concurrent processes, and I get panics in fyne. My understanding is that if I close a window in one goroutine, but refresh something else in another, it sometimes may cause a panic.

To mitigate the issue adding a "recover" to the places where it happens.

Feel free to suggest a better solution (I'll try to implement it) :)

Checklist:

andydotxyz commented 1 month ago

My understanding is that if I close a window in one goroutine, but refresh something else in another, it sometimes may cause a panic.

Not that I am aware. Anyhow panics should be resolved not worked around. Sorry you have found this bug, but what we need to do is fix the panic instead of hiding it. Hopefully you have a stack trace that we can use to help track it down?

xaionaro commented 1 month ago

[...] but what we need to do is fix the panic instead of hiding it.

I totally agree.

Hopefully you have a stack trace that we can use to help track it down?

It happens rarely and very difficult to reproduce, and unfortunately I do not have the stack traces anymore. These were pretty old fixes I made few months ago. Just I decided it is time to clean up the project and get rid of custom patchsets to other projects.

The rationale behind this PR was:

But I agree that one should address root causes and not hide bugs, so I was hesitant to publish this PR and mostly expected it to be closed without merging :)

andydotxyz commented 1 month ago

Thanks for understanding and discussing.

Bug#1: The fact that Close can panic at all (for example on a double call of UnboundedChan[T].Close) seems like a bug by itself

But isn't that what Go does for regular chan?

Bug#2: If the eventQueue is already closed then any event after that does not make any sense. I believe it is generally erroneous by design if a channel is used for writing by one entity (e.g. window.Close), but closed by another entity (gLDriver.runGL). So for example if I run runGL in one goroutine, but invoke window.Close from another, I can easily get a panic.

I think this is the bug you are fixing, yes. We should understand how it happens and resolve that. It may help to know that we are moving the threading model to a simpler setup in 2.6 which moves the event and render to the same goroutine - does that impact and/or resolve the root cause?

xaionaro commented 1 month ago

But isn't that what Go does for regular chan?

It does that for regular channels, but AFAIR it does not do that in Close functions, e.g. you can easily double-Close a file: https://play.golang.com/p/ydQoTGo_IlK

But I agree: IMO, even in this example is still a valid argument that double-close is generally a bug by itself and should be surfaced (not hidden). My opinionated rule of thumb is: surface as a log entry, but do not fail if unnecessary.

[...] moves the event and render to the same goroutine - does that impact and/or resolve the root cause?

Yeah, I think it should address the root cause and there will be no more panics (and no need for the fix I proposed in the PR). Thanks!

andydotxyz commented 1 month ago

It does that for regular channels, but AFAIR it does not do that in Close functions, e.g. you can easily double-Close a file: https://play.golang.com/p/ydQoTGo_IlK

Fair point, but I think the semantic of replacing a channel overrides the look of a file because of the presence of Close. We would have used close() if we could, but of course we cannot override these primitive functions.