Notification support #18
Conversation
|
Thanks for the feedback! I've addressed every issue/suggestion that you mentioned, let me know if I missed something. |
|
go test sometimes fails due to a data race on TestClientDisconnect but I have no idea how this could be fixed/improved. |
|
To you have the race details @irgendwr ? |
|
Ok I found the race in the logs, your concurrently accessing connected in multiple goroutines which is why you have the race. |
irgendwr
force-pushed
the
notification-support
branch
from
bd2b8d6
to
3a92186
Compare
irgendwr
force-pushed
the
notification-support
branch
from
388b79f
to
91a7399
Compare
|
The data race should be fixed for real now \o/ |
|
@stevenh Any feedback on the changes? |
| } else { | ||
| c.res = append(c.res, line) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Not sure what you mean. Timeout errors? Any errors are returned with c.err <- err
There was a problem hiding this comment.
yes, but as you're always reading, in order to facilitate notifications, and every read should have a timeout (not just the ones triggered by writes) then you need to deal with the expected timeouts otherwise the client would get a constant stream of errors even when they didn't send any commands.
There was a problem hiding this comment.
But why should the read in the message handler have a timeout? This would just lead to constant timeout errors and serve no purpose imho.
Otherwise I could set a deadline and and just ignore all of the errors caused by timeouts.
|
Happy new year to pull request #18 🎉 |
|
So can someone merge this ? |
|
Yeah, I would also like to see this getting merged. It honestly really sucks that after putting in a fair amount of effort into this he doesn't even respond anymore. I might just continue developing this in my own fork and give up on submitting any pull-requests here. I'm definitely not as skilled/experienced as @stevenh in go but otherwise it's impossible to get stuff implemented. |
|
Many apologies for dropping the ball on this one, I think I've been missing notifications from github. Had the issue before and had to have them flush their mail servers to fix it. Most the of issues are sorted so going to just merge and we can iterate if needed. |
This pull-request adds support for notifications (fixes #17).