StackExchange / StackExchange.Redis

General purpose redis client
https://stackexchange.github.io/StackExchange.Redis/
Other
5.88k stars 1.51k forks source link

Reading is not allowed after reader was completed #2692

Open nathanmascitelli opened 6 months ago

nathanmascitelli commented 6 months ago

Hi,

We are getting the following exception intermittently in production:

at System.IO.Pipelines.ThrowHelper.ThrowInvalidOperationException_NoReadingAllowed()
at System.IO.Pipelines.Pipe.AdvanceReader(SequencePosition& consumed, SequencePosition& examined)
at System.IO.Pipelines.Pipe.DefaultPipeReader.AdvanceTo(SequencePosition consumed, SequencePosition examined)
at StackExchange.Redis.PhysicalConnection.ReadFromPipe() in /_/src/StackExchange.Redis/PhysicalConnection.cs:line 1850

When this happens its alway after we have called IDatabase.StreamAddAsync.

Looking at PhysicalConnection I'm wondering if there is a race condition between ReadFromPipe and something calling Dispose? image

Does that make sense? I'm pretty sure this class is used by multiple threads and so I think the above is posslbe. Looking at the exception handler it seems like we don't even really want to report this exception? image

So to sum up:

We have seen this happen on multiple OS, dotnet, and package versions:

Let me know if you need more info.

Thanks!

NickCraver commented 5 months ago

It sounds like the connection is being used post-disposal here (at which point we shouldn't be sending any more commands) - is that accurate?

nathanmascitelli commented 5 months ago

@NickCraver so the application has one instance of IConnectionMultiplexer which is used by multiple threads. At some point one of the threads will dispose the connection (either the app is shutting down or some configruation has changed) yes. When that happens any other threads thats currently trying to use the IConnectionMultiplexer gets this error, always when trying to write a value to Redis.

In the screenshot above it looks like we actualy handle this OK when reading but if isReading is false then we will always report an exception. Maybe we just need to change that part?

nathanmascitelli commented 2 months ago

Any thoughts on my last comment?

mukhamedyarov commented 2 days ago

Hello, I am getting the same exceptions. Are there any updates on this?

NickCraver commented 1 day ago

Overall here: either do not dispose the multiplexer if you're still using it or stop using it before disposal - this advice is universal to all disposals in .NET. When you dispose an object, that's very intentionally when we cleanup resources and shut things down - if something in in-flight, that's an ill-advised thing to do.

nathanmascitelli commented 1 day ago

@NickCraver in general I agree with you, and ultimatly we updated our code to correctly recognize that this exception is equivalent to an ObjectDisposedException and act accordingly. However, I still wonder if the catch block should be tweaked so that regardless of if a read is in progress or not we consistanltly report and exception or not if we are in ReadFromPipe and the pipe is disposed. It seems odd to me (though maybe I'm just dumb) that sometimes we would report and exception and sometimes not.