feat: Robust connection handling, structured logging, and keepalive improvements (v1.5.0) #63
Conversation
|
Thanks, but we need a simple logger interface, for people who want to use another logger like uber zap or want to change the logging format etc. And i don't understand why we dereference the rc logger everywhere?: logger := rc.Logger there should be a more elegant solution |
|
Made a few changes to remedy that. The default is slog with a simple interface exposed for users to hook in their own loggers. |
|
LGTM, please give me some time to review friendly ping @sknr |
|
Sounds good, I’m in no rush, already using my fork for the time being. Thanks! |
|
@loeffel-io Sure, will have a look at it. |
recws.go
Outdated
| } | ||
| if !rc.getNonVerbose() { | ||
| log.Printf("Dial: connect handler was successfully established with %s\n", rc.url) | ||
| rc.Logger.Error("connect handler failed", "error", err) |
There was a problem hiding this comment.
@okaris Before your changes, the app exits with an error. I would suggest a similar behaviour if the SubscribeHandler fails, or at least triggering a CloseAndReconnect(). @loeffel-io Also the wording is a little bit confusing. SubscribeHandler but log message says connect handler. Let's stick to either one. What do you think?
recws.go
Outdated
| } | ||
| if err != nil { | ||
| rc.Logger.Error("write message error", "error", err) | ||
| rc.Logger.Info("closing connection and reconnecting") |
There was a problem hiding this comment.
You can remove this line, because of duplication in CloseAndReconnect()
recws.go
Outdated
| // Wait for either connection success or handshake timeout | ||
| select { | ||
| case <-connected: | ||
| rc.Logger.Info("connection established") |
There was a problem hiding this comment.
Can be removed since it duplicates logging message in line 470.
|
@sknr did you run a e2e test? |
I didn’t write comprehensive end-to-end tests, but I did implement some basic tests to cover key scenarios like reconnection handling, handshake timeouts, and send/receive behavior. These helped me verify core functionality and also led me to identify issues like duplicated log messages. |
|
ping @okaris |
|
3 participants
This PR introduces significant improvements to the recws library:
Key Changes:
Breaking Changes:
Most likely fixes all:
#57
#20
#43
#42
#39
#38
#20
#19
#61