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

Enable calling mirrorPool reader Read after EOF #411

Merged
merged 5 commits into from
Dec 15, 2021

Conversation

samutamm
Copy link
Contributor

pkg/filter/proxy/masterslavereader.go Outdated Show resolved Hide resolved
pkg/filter/proxy/proxy.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2021

Codecov Report

Merging #411 (bb08248) into main (327b43b) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #411      +/-   ##
==========================================
- Coverage   80.37%   80.25%   -0.12%     
==========================================
  Files          62       62              
  Lines        7107     7112       +5     
==========================================
- Hits         5712     5708       -4     
- Misses       1100     1107       +7     
- Partials      295      297       +2     
Impacted Files Coverage Δ
pkg/filter/proxy/primarysecondaryreader.go 86.53% <100.00%> (ø)
pkg/filter/proxy/proxy.go 78.64% <100.00%> (ø)
pkg/util/timetool/distributedtimer.go 95.45% <0.00%> (-4.55%) ⬇️
pkg/object/mqttproxy/client.go 79.87% <0.00%> (-1.22%) ⬇️
pkg/cluster/cluster.go 49.57% <0.00%> (-1.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e2b13c...bb08248. Read the comment docs.

@suchen-sci
Copy link
Contributor

Emmmm, based on golang side, "Looks like it's transferWriter probing to determine if it should send a chunked request", which means transferWriter may resend the request and call Read again. If that is true, will that means two goroutines call Read under high QPS? Do we need to use sync.Once? So, let me do some experiments about this....

@localvar
Copy link
Collaborator

@suchen-sci , I don't think two goroutines could call Read concurrently, but if it do happens, I'm sure it is a bug of the Go standard library, because io.Reader.Read is not required to be go-routine safe.

And as I mentioned in the slack discussion, if there are concurrent calls, use sync.Once cannot fix the issue either.

@suchen-sci
Copy link
Contributor

@localvar yeah, you are right. Golang use a writeCh to write these request, although they resend request during high QPS, it still keep thread-safe. So, sync.Once is unnecessary. But I still think Read after EOF in standard lib is wrong. If there are request body, first time it read the body and reach EOF. Second time, transferWriter do resend for chunked request (of course this is a hypothesis from golang issue comments), and read body only get EOF and empty body... And in source code, i don't find any place to re-read after receive EOF... But anyway, it is goroutine-safe.

@xxx7xxxx xxx7xxxx merged commit c08f411 into easegress-io:main Dec 15, 2021
@samutamm samutamm deleted the fix-reader branch December 15, 2021 03:29
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.

Proxy with mirrorPool panics under high QPS
5 participants