Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent panic: send on closed channel during termination #15

Merged
merged 1 commit into from
Apr 2, 2019

Conversation

azr
Copy link
Contributor

@azr azr commented Mar 22, 2019

sometimes it's possible to receive a SIGWINCH right in between when close(tty.ss) and close(tty.ws) are being called, causing a panic.

Making the WINSIZE production goroutine close the chan will prevent this.

sometimes it's possible to receive a SIGWINCH right in between when `close(tty.ss)` and `close(tty.ws)` are being called, causing a panic.

Making the `WINSIZE` production goroutine close the chan will prevent this.
@srlehn
Copy link
Contributor

srlehn commented Mar 31, 2019

demo code here: #19

@srlehn
Copy link
Contributor

srlehn commented Mar 31, 2019

required by PR: gizak/termui#233

@mattn
Copy link
Owner

mattn commented Mar 31, 2019

I don't think this is right since ws must trigger several time.

@srlehn
Copy link
Contributor

srlehn commented Mar 31, 2019

I only know that this definitely fixes the crashes for me.
The crash happens spontaneously after resizing for a while.

@azr
Copy link
Contributor Author

azr commented Apr 1, 2019

Hey @mattn, ss takes all sigwinch syscalls but ss must be depleted before we close ws, otherwise panic: send on a closed channel. The PR ensures ss is depleted before we close ws since the close will only happen after the range.

defer close(tty.ws)
for sig := range tty.ss {

@mattn
Copy link
Owner

mattn commented Apr 2, 2019

I just understood what is happening with your explanation.

@mattn mattn merged commit 76a2065 into mattn:master Apr 2, 2019
@mattn
Copy link
Owner

mattn commented Apr 2, 2019

Thank you

@azr
Copy link
Contributor Author

azr commented Apr 2, 2019

Thanks !! Sorry, my first explanation was not the clearest 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants