Replace various update channels with one unique channel #137
Conversation
Codecov Report
@@ Coverage Diff @@
## v1 #137 +/- ##
========================================
- Coverage 74.5% 74.5% -0.01%
========================================
Files 31 31
Lines 3271 3251 -20
========================================
- Hits 2437 2422 -15
+ Misses 550 544 -6
- Partials 284 285 +1
Continue to review full report at Codecov.
|
client/client.go
Outdated
| go func() { | ||
| c.Errors <- resp | ||
| c.Updates <- resp |
There was a problem hiding this comment.
I don't think it's legitimate to put this in a separate go-routine: if sending to the channel blocks and multiple goroutines exist trying to send different items, isn't the channel item ordering undefined again?
I think the library has to block here, and the user must be responsible for consuming from the channel in parallel with the function call (or somehow be very confident their channel buffer size is large enough to avoid deadlock).
There was a problem hiding this comment.
(this comment applies throughout this PR)
client/client.go
Outdated
| // A channel to which unilateral updates from the server will be sent. An | ||
| // update can be one of: *imap.StatusResp, *imap.MailboxStatus, *imap.Message, | ||
| // *ExpungeUpdate. Note that blocking this channel blocks the whole client, | ||
| // so it's recommended to use a buffered channel. |
There was a problem hiding this comment.
I think the IDLE command is an example where the number of updates that will be received may not be knowable in advance; in this case there is a risk of filling the channel and deadlock occuring if the channel is not emptied in parallel. I think the best recommendation would be for the channel to be consumed in parallel from a separate goroutine during execution of a request, (and for the user to ensure that the channel is emptied once the request returns, so that any updates have been synced to the local state).
There was a problem hiding this comment.
Yeah, that's a good point, I changed the docs. In general, it's unsafe to rely on the server only sending a given number of responses and it's better to use separate goroutines.
|
Looks good - many thanks @emersion! |
Updatestruct per update type?@jim-minter, what do you think?
Fixes #136