air-verse / air

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

fix: fix proxy error handling and race conditions #585

Closed ndajr closed 1 month ago

ndajr commented 1 month ago

I improved the error handling by avoiding log.Fatal as much as possible which doesn't clean up the resources properly (leaving the process and ports open). I couldn't reproduce this issue with the steps mentioned but after running go test -v -count=1 -race -run TestProxyStream ./runner a few times I got some race condition warnings. You can run those tests to validate the new behaviour working as expected with no warnings. I also added a CORS wildcard allow asked in another issue.

xiantang commented 1 month ago

hey, coverage is required. currently I wanna use "github.com/bytedance/mockey" to mock the filesystem event. because the old style ut is unstable.

xiantang commented 1 month ago

and i'm not sure, Is this panic caused by race conditions?

ndajr commented 1 month ago

This is a normal flow

  1. the user runs air with proxy enabled and access the proxy via browser
  2. the script is injected on the page
  3. the browser connects to /internal/reload endpoint (server-sent events)
  4. A new subscriber is created on proxy_stream
  5. The connection with /internal/reload stays alive until the user closes the tab, causing the connection to be closed (context Done signal is sent) and we remove the subscriber in the subscribers map

If the user opened two tabs accessing the proxy, we would have 2 subscribers, and if suddenly the browser is closed those two subscriptions will be removed from the map, but a map is not thread safe, that's why I am moving from sync.Mutex (which prevents race condition with reads) to sync.RWMutex (prevents race condition with reads and writes), and also using an atomic counter which is recommended when dealing with multiple go routines (https://gobyexample.com/atomic-counters). I hope this gives an explanation on why I think the current code can cause panics when a race condition happens and I could validate it running the tests against the master branch and this branch (go test -v -count=1 -race -run TestProxyStream ./runner).

ndajr commented 1 month ago

Regarding the coverage, I will see if I can improve the tests for proxy. But for engine.go it decreased -0.17% just by checking the proxy error and failed the job. Is there a way we can decrease the minimum coverage threshhold? Otherwise I will have to add tests for functions that are not proxy related

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 81.25000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 68.89%. Comparing base (3f19370) to head (d10e7de). Report is 2 commits behind head on master.

Files Patch % Lines
runner/proxy.go 76.47% 4 Missing :warning:
runner/engine.go 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #585 +/- ## ========================================== + Coverage 67.34% 68.89% +1.55% ========================================== Files 12 11 -1 Lines 1078 1061 -17 ========================================== + Hits 726 731 +5 + Misses 264 243 -21 + Partials 88 87 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

xiantang commented 1 month ago

Regarding the coverage, I will see if I can improve the tests for proxy. But for engine.go it decreased -0.17% just by checking the proxy error and failed the job. Is there a way we can decrease the minimum coverage threshhold? Otherwise I will have to add tests for functions that are not proxy related

UT is fine. because u already covered enough