SkyAPM / go2sky

Distributed tracing and monitor SDK in Go for Apache SkyWalking APM
https://skywalking.apache.org/
Apache License 2.0
448 stars 123 forks source link

fix: When serves both http&websocket,ws connection denied with "bad handshake". #160

Closed nealwon closed 2 years ago

nealwon commented 2 years ago

When using package gorilla/websocket, calls Upgrader.Upgrade(w http.ResponseWriter, r *http.Request, responseHeader http.Header),it asserts the ResponseWriter to be a Hijacker implementation, if assertion fails ws connection will be denied.

Client error: websocket: bad handshake Server error: websocket: response does not implement http.Hijacker

gorilla/websocket code details:server.go

173     h, ok := w.(http.Hijacker)
174     if !ok {
175         return u.returnError(w, r, http.StatusInternalServerError, "websocket: response does not implement http.Hijacker")
176     }
wu-sheng commented 2 years ago

Please update the title and contents in English.

arugal commented 2 years ago

@nealwon I need to do another PR fix CI, please wait :)

codecov-commenter commented 2 years ago

Codecov Report

Merging #160 (11cb393) into master (6492c49) will decrease coverage by 0.11%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   64.64%   64.53%   -0.12%     
==========================================
  Files          22       22              
  Lines        1089     1094       +5     
==========================================
+ Hits          704      706       +2     
- Misses        330      334       +4     
+ Partials       55       54       -1     
Impacted Files Coverage Δ
plugins/http/server.go 42.10% <0.00%> (-4.05%) :arrow_down:
sampler.go 81.96% <0.00%> (+3.27%) :arrow_up:

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 6492c49...11cb393. Read the comment docs.

wu-sheng commented 2 years ago

Could you share how this case happens?

gorilla/websocket seems a 3rd pary framework, rather than this one bundled with go core lib only. If you plan to support this new library, you should land your codes to go2sky-plugins repository.

nealwon commented 2 years ago

Yes,it's a popular 3rd-part lib. In my project, I use this lib(gorilla/websocket) and want to try to use skywalking as performance monitoring.When i load go2sky lib, service start up normally,but all clients cannot connect to server due to "bad handshake". Non-core libs not supported by go2sky,I can use fork to test skywalking.

Now i'll close this PR!

wu-sheng commented 2 years ago

I mean this should be a plugin, not a core lib change. You could create another client to inherit this and implement a new interface, which would be more nice.

nealwon commented 2 years ago

OK, thx.