Calling static asynchronous methods from an ASP.net project

Question

I am wondering if this scenario will be thread safe and if there are problems that I don’t see right now:

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

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

• Finally, the static method does some work (which requires userId)

I believe this approach is thread safe, and everything will be done correctly if two users call this method at the same time (say, in the same nanosecond). Am I right or wrong? If I am mistaken, what would be the correct way to use static methods in an ASP.net project?

EDIT

Here is the code :)

This is the controller call:

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

Here is what 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 see, DeleteWorkoutByIdAsync calls the 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 the 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

"Safe flow" is not an independent term. Is Thread Safe in the Face of What? What parallel changes do you expect here?

Let's look at several aspects:

• Your own mutable general state: you do not have a general state in this code; therefore, it is automatically thread safe.
• Indirect split state: DatabaseContext . This is similar to the sql database, and they are usually “safe” in the stream, but what exactly this means depends on the particular database. For example, you delete the Trenings line, and if any other thread also deletes the same line, you are likely to get a (safe) concurrency violation exception. And depending on the isolation level, you may get concurrency violation exceptions even for other specific mutations of “Trenings”. In the worst case, this means one failed request, but the database itself will not be corrupted.
• Redis is essentially single-threaded, so all operations are serialized and in this sense "thread safe" (which may not buy you a lot). Your deletion code receives a set of keys, and then deletes no more than one of them. If two or more threads try to delete the same key at the same time, it is possible that one thread will try to delete the nonexistent key, and this may be unexpected for you (but this will not damage the DB).
• Implicit consistency between redis + sql: it looks like you are using hints, so the chances of unrelated conflicts of things are small. Your example only contains a delete operation (which probably does not cause consistency issues), so it’s hard to imagine if, under all other circumstances, redis and the sql database will remain unchanged. In the general case, if your identifiers are never reused, you are likely to be safe - but synchronizing the two databases is a serious problem and you are unlikely to make a mistake.

However, your code seems overly complex for what it does. I would recommend that you greatly simplify it if you want to keep it in the long run.

• Do not use ref parameters unless you really know what you are doing (and this is not necessary here).
• Do not mix 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 crash when something unexpected happens, because code that is "chromating" can be almost impossible to debug (and you will write errors).
• It is not efficient to return an array of tasks if the only thing you can really do is wait for them all - it can also do this in calllee to simplify the API.
• Do not use redis. This is probably just a distraction - you already have a different database, so you are unlikely to need it here, with the exception of performance reasons, and it is extremely early to add additional database mechanisms for a hypothetical performance problem. There is a reasonable chance that the extra overhead that requires additional connections can make your code slower than if you only had one bit, especially if you cannot save many sql queries.

Answer 2

Note: this answer was posted before the OP changed its question to add its own code, indicating that this is actually a question of whether async / await is thread safe.

---

Static methods alone are not a problem. If the static method is autonomous and manages to do its job using only local variables, then it is well protected by the stream.

Problems arise if the static method is not self-sufficient (delegates unsafe code), or if it controls the static state in an unsafe way, that is, it accesses static variables both for reading and writing outside of lock() .

For example, int.parse() and int.tryParse() are static but perfectly thread safe. Imagine the horror if they are not thread safe.

Answer 3

what you do here is synchronized in the list (deleteTasks). If you do this, I would recommend 1 out of 2 things.

1) Either use thread-safe collections https://msdn.microsoft.com/en-us/library/dd997305(v=vs.110).aspx

2) Let your DeleteItemFromRedisAsync return the task and wait for it.

Although I think that in this particular case I do not see any problems, as soon as you reorganize it, and DeleteItemFromRedisAsync can be called several times in parallel, then you will have problems. The reason is that if several threads can change your list of remote tasks, then you are no longer guaranteed that you will collect them ( https://msdn.microsoft.com/en-us/library/dd997373(v=vs.110) .aspx , if 2 threads do "Add" / "Add to the end" in a safe way without threads, while 1 of them is lost), so you could skip the task, waiting for the completion of all these actions.

I would also avoid mixing paradigms. Either use async / await or keep track of the collection of tasks and add methods to this list. do not do both. This will help maintain your code in the long run. (note that threads can return the task, you collect them, and then wait for them all, but then the collection method is responsible for any problems with the threads, and is not hidden in the called method)