WIP: #969 testable ChannelMessageQueue #1000
Conversation
| /// <param name="incomingValues">A channel reader representing the incoming values.</param> | ||
| /// <param name="onUnsubscribe">A delegate to call when <see cref="Unsubscribe(CommandFlags)"/> is called.</param> | ||
| /// <param name="onUnsubscribeAsync">A delegate to call when <see cref="UnsubscribeAsync(CommandFlags)"/> is called.</param> | ||
| /// <param name="onInternalError">REVIEW: Need more context here</param> |
There was a problem hiding this comment.
I'm not super clear on the purpose of this onInternalError callback (represents the previous call directly to ConnectionMultiplexer.OnInternalError). It was only called when a non-ChannelClosedException was thrown by ReadAsync in the message loop (which is highly unlikely).
There was a problem hiding this comment.
it was simply to do something in the case that an unexpected exception happened - agree "highly unlikely", but: at least in the "real" code, should do something; probably doesn't need to be mockable, if it presents a problem
There was a problem hiding this comment.
We could have two constructors. One that takes a RedisSubscriber (i.e. the internal one, that can access OnInternalError directly and one that just takes a ChannelReader and an action to fire when unsubscribe is called. Since this is primarily (exclusively?) for testing, it's less important that the testable entry point be able to perform all the necessary functions.
| catch (ChannelClosedException) { break; } // expected | ||
| catch (Exception ex) | ||
|
|
||
| // Keep trying to read values |
There was a problem hiding this comment.
This is the pattern we generally use to read from a channel and gracefully stop without an exception. If there's a reason for the older behavior, I'm happy to reset it.
|
|
||
| if (Completion.IsFaulted) | ||
| { | ||
| _onInternalError(Completion.Exception.InnerException); |
There was a problem hiding this comment.
This is technically new behavior, it will trigger this callback if the channel is closed with an Exception, but we don't ever do that in the normal flow. A custom implementor that uses their own Channel in ChannelMessageQueue might though...
| { | ||
| return false; | ||
| } | ||
| // REVIEW: Need more context here to properly replace this. |
There was a problem hiding this comment.
I think this can be easily restored by moving it to RedisSubscriber, since that's where the handler lives now. Just haven't done that yet.
Fixes #969
This PR sketches out what it would look like to make
ChannelMessageQueueconstructable by a test, so that mock implementations ofISubscribercould use it.It could also be useful outside of a test, since
ISubscriberis a public interface and (in 2.0) has methods that an external assembly can't implement because it is unable to constructChannelMessageQueueI believe this to be non-breaking, but there are some quirks about it. Comments below.
cc @BrennanConroy @davidfowl