Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unobserved task exceptions when using ScriptEvaluateAsync in a batch #2651

Open
ProffP opened this issue Feb 23, 2024 · 7 comments
Open

Unobserved task exceptions when using ScriptEvaluateAsync in a batch #2651

ProffP opened this issue Feb 23, 2024 · 7 comments

Comments

@ProffP
Copy link

ProffP commented Feb 23, 2024

I have run into an issue where work started in a batch is not observable without having a try catch around batch.Execute and then awaiting the tasks individually.

My test setup is running the following code and then stopping the redis server.

I would have expected that batch.Execute would have stopped any work from continuing if it failed itself.

using StackExchange.Redis;

internal class Program
{
    private static readonly LuaScript EnsureTopicScript = LuaScript.Prepare(
        @"
            redis.call('PUBLISH', @PublishTopic, @Message)
        ");

    public static async Task Main(string[] args)
    {
        TaskScheduler.UnobservedTaskException += (sender, eventArgs) =>
        {
            Console.WriteLine("Unobserved exception");
        };
        var connection = await ConnectionMultiplexer.ConnectAsync("localhost:6379");
        Console.WriteLine("Connected");
        var db = connection.GetDatabase();
        var i = 0;
        while (true)
        {
            var batch = db.CreateBatch();
            var t = batch.PublishAsync("Channel", i++, CommandFlags.FireAndForget);
            var t2 = batch.PublishAsync("Channel2", i, CommandFlags.FireAndForget);
            var t3 = batch.ScriptEvaluateAsync(EnsureTopicScript, new
            {
                PublishTopic = "Channel3",
                Message = i
            });
            try
            {
                //try
                //{
                    batch.Execute();
                //}
                //catch(Exception ex) { }
                await Task.WhenAll(t, t2, t3);
            }
            catch (Exception ex)
            {

            }
        }
    }
}
@mgravell
Copy link
Collaborator

what is the scenario where this repros? the code as shown just runs happily

@ProffP
Copy link
Author

ProffP commented Feb 23, 2024

Hey, when I run this code and then stop the redis server, then I get "Unobserved exception" printed continuously.

@mgravell
Copy link
Collaborator

ah, sorry; I missed the "stop the redis server" step! one moment

@mgravell
Copy link
Collaborator

OK; so... in a way, it is correct: an exception occurred, and you didn't observe it. This is specifically referring to the t3 line, and if we add FireAndForget to t3: we don't see the UTE.

But the better question, perhaps, is should this happen; it is a complex one - I can see merit in "not". In theory we can observe the exception automatically. We could also try marking the inner operations as "cancelled" rather than "faulted".

One for discussion next sync, @NickCraver @philon-msft ?

@ProffP
Copy link
Author

ProffP commented Feb 23, 2024

Hey, thanks for the prompt response.
Yeah for my purposes I am okay with any approach, this was just a bug that I found in our solution which was a bit cryptic to locate as I had an incorrect assumption on how to use batches in redis.
I personally like the idea of setting the tasks to cancelled because it would simplify the logic;
Just to be 100% sure of my assumptions here. If I ignore the batch.Execute() exceptions, will all cases result in an exception being thrown by the tasks themselves? Because if not, then I will need to make logic to catch the exception on batch.Execute. Then await the tasks.

Note: in our solution we have unobserved exceptions rethrowing on the main thread, halting the application.

@mgravell
Copy link
Collaborator

fundamentally, if you're running multiple things concurrently (which is what appears to be the case here) and you absolutely care that all are observed (which is more of a concern on .NET Framework than later versions of .NET), then you need to be careful - let's assume the batch succeeds - we could still have 4 inner tasks, A B C and D, and if you await them in turn, imagine that B throws - if C or D also throws, it will go unobserved.

Are you on .NET Framework here? Note also: batches often aren't as useful as you might expect - note that you could probably remove the batch completely here, and just invoke the commands separately. If it was a transaction (MULTI/EXEC) it would be different, but honestly: I tend to advise people to use Lua in place of MULTI/EXEC.

@ProffP
Copy link
Author

ProffP commented Feb 23, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants