Websocket #411
Websocket #411
Conversation
Codecov ReportPatch coverage is ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
📢 Thoughts on this report? Let us know!. |
|
This PR is ready for review. |
notify/notify.go
Outdated
| @@ -50,6 +51,7 @@ type Config struct { | |||
| Sms []sms.NotifyConfig `yaml:"sms,omitempty" json:"sms,omitempty" jsonschema:"title=SMS Notification,description=SMS Notification Configuration"` | |||
| Teams []teams.NotifyConfig `yaml:"teams,omitempty" json:"teams,omitempty" jsonschema:"title=Teams Notification,description=Teams Notification Configuration"` | |||
| Shell []shell.NotifyConfig `yaml:"shell,omitempty" json:"shell,omitempty" jsonschema:"title=Shell Notification,description=Shell Notification Configuration"` | |||
| WebSocket []websocket.WebSocket `yaml:"webSocket,omitempty" json:"webSocket,omitempty" jsonschema:"title=WebSocket Notification,description=WebSocket Notification Configuration"` | |||
There was a problem hiding this comment.
to align with other items and the documentation.
| WebSocket []websocket.WebSocket `yaml:"webSocket,omitempty" json:"webSocket,omitempty" jsonschema:"title=WebSocket Notification,description=WebSocket Notification Configuration"` | |
| WebSocket []websocket.WebSocket `yaml:"websocket,omitempty" json:"websocket,omitempty" jsonschema:"title=WebSocket Notification,description=WebSocket Notification Configuration"` |
There was a problem hiding this comment.
Removed this line. This was a wrong copy/paste. websocket is a probe, not a notify module.
probe/websocket/ws.go
Outdated
| } | ||
|
|
||
| begin := time.Now() | ||
| remaining := new(time.Duration) |
There was a problem hiding this comment.
why declare remaining as a pointer?
docs/Manual.md
Outdated
| ```yaml | ||
| websocket: | ||
| name: asr-server | ||
| url: wss://example.com/asr/ | ||
| timeout: 5s | ||
| ``` |
There was a problem hiding this comment.
In Conf, websocket is an array []websocket.WebSocket. And in examples above, http, tcp use array yaml format. Should this example update to array format?
websocket:
- name: ...
...
- name: ...
...In WebSocket struct, there are field of proxy and headers, could you please add some example to them? Please make sure every parameter in the configuration has their own example, so users know how to use it properly. Please check 1.2.2 Complete Configuration of http.
| "github.com/megaease/easeprobe/probe/base" | ||
| ) | ||
|
|
||
| type WebSocket struct { |
There was a problem hiding this comment.
Please add comments to exportable struct and methods in this file. Websocket, Config, DoProbe.
probe/websocket/ws.go
Outdated
|
|
||
| // doing nothing else but trigger the read message loop to receive the | ||
| // Pong and Close messages from the server. exit after ws.Close() | ||
| wg := sync.WaitGroup{} |
There was a problem hiding this comment.
I am a little confused about this WaitGroup here. wg should be used to wait the goroutine is finished. Then maybe do some checking after. But in this case, pingPongChan is used to do the waiting. So, maybe remove this wg?
There was a problem hiding this comment.
Ideally the wg.Wait is unnecessary. the goroutine will quit shortly. In case of any extreme condition, the ws connection hangs, doProbe will hang too and not return, to avoid goroutine leak. I was a little knotty when adding the wg. Wait.
probe/websocket/ws.go
Outdated
| remaining = h.ProbeTimeout - time.Now().Sub(begin) | ||
| err = ws.WriteControl(websocket.PingMessage, []byte{}, time.Now().Add(remaining)) | ||
| if err != nil { | ||
| ws.Close() |
There was a problem hiding this comment.
ws.Close is called before with defer ws.Close(), so no need to call it again. And since ws.Close is called in defer, then the goroutine before will return too. So, no goroutine leak.
probe/websocket/ws.go
Outdated
| } | ||
| }() | ||
|
|
||
| remaining = h.ProbeTimeout - time.Now().Sub(begin) |
There was a problem hiding this comment.
It's better to use time.Since instead of time.Now().Sub()...
There was a problem hiding this comment.
should use time.Since instead of time.Now().Sub (S1012)go-staticcheck
probe/websocket/ws.go
Outdated
| return false, "ping timeout" | ||
| case <-pingPongChan: | ||
| remaining = h.ProbeTimeout - time.Now().Sub(begin) | ||
| closeCode := bytes.NewBufferString(fmt.Sprintf("%d", websocket.CloseNormalClosure)) |
There was a problem hiding this comment.
Please use websocket.FormatCloseMessage() to format close message. FormatCloseMessage treat CloseNormalClosure code as uint16 with two bytes. Here, it is a four byte string...
No description provided.