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

[3.3] Fix triple rest http GET failed #14223

Merged
merged 1 commit into from
May 21, 2024
Merged

[3.3] Fix triple rest http GET failed #14223

merged 1 commit into from
May 21, 2024

Conversation

oxsean
Copy link
Collaborator

@oxsean oxsean commented May 20, 2024

What is the purpose of the change

HTTP/1 upgrade caused GET request to failed
Case:
https://github.com/apache/dubbo-samples/blob/master/2-advanced/dubbo-samples-triple-rest/dubbo-samples-triple-rest-springmvc/src/test/java/org/apache/dubbo/rest/demo/test/ConsumerIT.java#L47

curl.exe -v --http2 http://127.0.0.1:50052/demo/hello?name=world

Expect 'Hello world' but respone empty body.

Root cause

HttpServerUpgradeHandler.java#L377 already support write response, HttpServerAfterUpgradeHandler duplicate writes cause incorrect GET results.

Brief changelog

Remove unnecessary HttpServerAfterUpgradeHandler

@oxsean oxsean changed the title [3.3] Fix http GET failed cause by http1 upgrade cause in triple [3.3] Fix triple rest http GET failed May 20, 2024
Copy link

sonarcloud bot commented May 20, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@walklown
Copy link
Contributor

I think you are right. I seem to be confused by another bug. Anyway 'HttpServerAfterUpgradeHandler' can be removed now, I will keep an eye on it.

@AlbumenJ AlbumenJ merged commit 80ffcd7 into apache:3.3 May 21, 2024
19 checks passed
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.

3 participants