search

air-verse / air

☁️ Live reload for Go apps
GNU General Public License v3.0
16.32k stars 770 forks source link

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

Closed davidovich closed 1 month ago

davidovich commented 1 month ago

To reproduce:

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Hi @xiantang, here it is: https://github.com/cosmtrek/air/pull/585

davidovich commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

should be fixed in https://github.com/cosmtrek/air/pull/585. will release in next version of air