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

fix: fix proxy error handling and race conditions #585

Merged
merged 5 commits into from
May 26, 2024

Conversation

ndajr
Copy link
Contributor

@ndajr ndajr commented May 14, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@ndajr
Copy link
Contributor Author

ndajr commented May 24, 2024

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).

runner/proxy_stream.go Outdated Show resolved Hide resolved
@ndajr
Copy link
Contributor Author

ndajr commented May 24, 2024

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

Copy link

codecov bot commented May 24, 2024

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 ⚠️
runner/engine.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xiantang
Copy link
Collaborator

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

@xiantang xiantang merged commit f673320 into air-verse:master May 26, 2024
9 checks passed
@ndajr ndajr deleted the fix-live-proxy branch May 26, 2024 07:21
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.

2 participants