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

panic when shutting down with SIGINT signal if proxy is enabled #584

Closed
davidovich opened this issue May 12, 2024 · 8 comments
Closed

panic when shutting down with SIGINT signal if proxy is enabled #584

davidovich opened this issue May 12, 2024 · 8 comments

Comments

@davidovich
Copy link

To reproduce:

  • Start air with proxy enabled in config.
  • Type CTRL-C

Result:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x8 pc=0x100e5b604]

goroutine 5074 [running]:
github.com/cosmtrek/air/runner.(*ProxyStream).RemoveSubscriber(0x14000236588, 0x9b)
        /Users/[user]/go/pkg/mod/github.com/cosmtrek/air@v1.52.0/runner/proxy_stream.go:42 +0xc4
github.com/cosmtrek/air/runner.(*Proxy).reloadHandler.func1()
        /Users/[user]/go/pkg/mod/github.com/cosmtrek/air@v1.52.0/runner/proxy.go:159 +0x84
created by github.com/cosmtrek/air/runner.(*Proxy).reloadHandler in goroutine 4997
        /Users/[user]/go/pkg/mod/github.com/cosmtrek/air@v1.52.0/runner/proxy.go:157 +0x2b8

A theory is that proxy.Stop() is called before the <- ctx.Done() channel yields, allowing making a call on an empty value of the map.

@ndajr
Copy link
Contributor

ndajr commented May 13, 2024

Hi @davidovich, I am the author of the proxy feature. I am not able to reproduce this issue with the steps you mentioned. What is your OS and Go version? Does this happen when you start air for the first time with proxy enabled, or after a few reload events?

@ndajr
Copy link
Contributor

ndajr commented May 14, 2024

Could you test please my branch fix-live-proxy? https://github.com/cosmtrek/air/compare/master...ndajr:air:fix-live-proxy?expand=1. I've improved the error handling, used atomic counters and RWMutex which should help to avoid race conditions

@xiantang
Copy link
Collaborator

Could you test please my branch fix-live-proxy? https://github.com/cosmtrek/air/compare/master...ndajr:air:fix-live-proxy?expand=1. I've improved the error handling, used atomic counters and RWMutex which should help to avoid race conditions

could u make a pr ?

@xiantang
Copy link
Collaborator

To reproduce:

  • Start air with proxy enabled in config.
  • Type CTRL-C

Result:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x8 pc=0x100e5b604]

goroutine 5074 [running]:
github.com/cosmtrek/air/runner.(*ProxyStream).RemoveSubscriber(0x14000236588, 0x9b)
        /Users/[user]/go/pkg/mod/github.com/cosmtrek/air@v1.52.0/runner/proxy_stream.go:42 +0xc4
github.com/cosmtrek/air/runner.(*Proxy).reloadHandler.func1()
        /Users/[user]/go/pkg/mod/github.com/cosmtrek/air@v1.52.0/runner/proxy.go:159 +0x84
created by github.com/cosmtrek/air/runner.(*Proxy).reloadHandler in goroutine 4997
        /Users/[user]/go/pkg/mod/github.com/cosmtrek/air@v1.52.0/runner/proxy.go:157 +0x2b8

A theory is that proxy.Stop() is called before the <- ctx.Done() channel yields, allowing making a call on an empty value of the map.

plz provide OS version, and run air -v

@ndajr
Copy link
Contributor

ndajr commented May 14, 2024

Hi @xiantang, here it is: #585

@davidovich
Copy link
Author

davidovich commented May 14, 2024

On MacOS

GOARCH=arm64
GOOS=darwin

Built from a go install.

  __    _   ___  
 / /\  | | | |_) 
/_/--\ |_| |_| \_ v1.52.0, built with Go go1.22.0

If it matters, I run from make as a driver.

@davidovich
Copy link
Author

Another thing comes to mind.

Merely starting air and closing it with SIGINT doesn't trigger the panic, I think we actually need to serve some pages to the browser to observe the failure (to have multiple subscribers ?).

But now, I can't seem to reproduce the panic...

I did observe though that if you SIGINT the air process before saving any content, the managed executable stays running.

@xiantang
Copy link
Collaborator

should be fixed in #585. will release in next version of air

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

No branches or pull requests

3 participants