Calling static async methods from ASP.net project

Question

I'm wondering will this scenario be thread safe and are there issues that I'm not currently seeing:

1. From ASP.net controller I call non-static method from non-static class (this class is in another project, and class is injected into controller).

2. This method (which is non-static) does some work and calls some other static method passing it userId

3. Finally static method does some work (for which userId is needed)

I believe this approach is thread safe, and that everything will be done properly if two users call this method at the same time (let's say in same nanosecond). Am I correct or completely wrong ? If I am wrong what would be correct way of using static methods within ASP.net project ?

EDIT

Here is code :)

This is call from the controller:

await _workoutService.DeleteWorkoutByIdAsync(AzureRedisFeedsConnectionMultiplexer.GetRedisDatabase(),AzureRedisLeaderBoardConnectionMultiplexer.GetRedisDatabase(), workout.Id, userId);

Here how DeleteWorkoutByIdAsync looks like:

public async Task<bool> DeleteWorkoutByIdAsync(IDatabase redisDb,IDatabase redisLeaderBoardDb, Guid id, string userId)
    {

        using (var databaseContext = new DatabaseContext())
        {
            var workout = await databaseContext.Trenings.FindAsync(id);

            if (workout == null)
            {
                return false;
            }

            databaseContext.Trenings.Remove(workout);

            await databaseContext.SaveChangesAsync();

            await RedisFeedService.StaticDeleteFeedItemFromFeedsAsync(redisDb,redisLeaderBoardDb, userId, workout.TreningId.ToString());
        }

        return true;
    }

As you can notice DeleteWorkoutByIdAsync calls static method StaticDeleteFeedItemFromFeedsAsync which looks like this:

public static async Task StaticDeleteFeedItemFromFeedsAsync(IDatabase redisDb,IDatabase redisLeaderBoardDd, string userId, string workoutId)
 {


        var deleteTasks = new List<Task>();
        var feedAllRedisVals = await redisDb.ListRangeAsync("FeedAllWorkouts:" + userId);
        DeleteItemFromRedisAsync(redisDb, feedAllRedisVals, "FeedAllWorkouts:" + userId, workoutId, ref deleteTasks);


        await Task.WhenAll(deleteTasks);
  }

And here is static method DeleteItemFromRedisAsync which is called in StaticDeleteFeedItemFromFeedsAsync:

private static void DeleteItemFromRedisAsync(IDatabase redisDb, RedisValue [] feed, string redisKey, string workoutId, ref List<Task> deleteTasks)
  {
        var itemToRemove = "";

        foreach (var f in feed)
        {

            if (f.ToString().Contains(workoutId))
            {
                itemToRemove = f;
                break;
            }

        }
        if (!string.IsNullOrEmpty(itemToRemove))
        {
            deleteTasks.Add(redisDb.ListRemoveAsync(redisKey, itemToRemove));
        }
  }

Answer 1

"Thread safe" isn't a standalone term. Thread Safe in the the face of what? What kind of concurrent modifications are you expecting here?

Let's look at a few aspects here:

• Your own mutable shared state: You have no shared state whatsoever in this code; so it's automatically thread safe.
• Indirect shared state: DatabaseContext. This looks like an sql database, and those tend to be thread "safe", but what exactly that means depends on the database in question. For example, you're removing a Trenings row, and if some other thread also removes the same row, you're likely to get a (safe) concurrency violation exception. And depending on isolation level, you may get concurrency violation exceptions even for other certain mutations of "Trenings". At worst that means one failed request, but the database itself won't corrupt.
• Redis is essentially single-threaded, so all operations are serialized and in that sense "thread safe" (which might not buy you much). Your delete code gets a set of keys, then deletes at most one of those. If two or more threads simultaneously attempt to delete the same key, it is possible that one thread will attempt to delete a non-existing key, and that may be unexpected to you (but it won't cause DB corruption).
• Implicit consistency between redis+sql: It looks like you're using guids, so the chances of unrelated things clashing are small. Your example only contains a delete operation (which is likely no to cause consistency issues), so it's hard to speculate whether under all other circumstances redis and the sql database will stay consistent. In general, if your IDs are never reused, you're probably safe - but keeping two databases in sync is a hard problem, and you're quite likely to make a mistake somewhere.

However, your code seems excessively complicated for what it's doing. I'd recommend you simplify it dramatically if you want to be able to maintain this in the long run.

• Don't use ref parameters unless you really know what you're doing (and it's not necessary here).
• Don't mix up strings with other data types, so avoid ToString() where possible. Definitely avoid nasty tricks like Contains to check for key equality. You want your code to break when something unexpected happens, because code that "limps along" can be virtually impossible to debug (and you will write bugs).
• Don't effectively return an array of tasks if the only thing you can really do is wait for all of them - might as well do that in the callee to simplify the API.
• Don't use redis. It's probably just a distraction here - you already have another database, so it's very unlikely you need it here, except for performance reasons, and it's extremely premature to go adding whole extra database engines for a hypothetical performance problem. There's a reasonable chance that the extra overhead of requiring extra connections may make your code slower than if you had just one db, especially if you can't save many sql queries.