8

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));
        }
  }
Improve this question
10
  • 3
    I think you need to post the code. Whether something is thread safe (or not) depends on every line of code that could be executed. Commented Dec 3, 2015 at 15:46
  • 2
    If the static function manges to do all its work using local variables only, without making use of any static variables, then you are fine. Otherwise, you are not fine at all. But why don't you post some code for us to see? Commented Dec 3, 2015 at 15:50
  • 2
    @hyperN your edit changes everything. You are essentially asking whether async/await is thread-safe, which is a vastly different question requiring a far more complicated answer. Commented Dec 3, 2015 at 16:02
  • 2
    I wish I was knowledgeable enough about async/await to be able to answer your question. Unfortunately, I am not. And I am even afraid that even if you reason about it thoroughly, and come to the logical conclusion that it should be okay, you probably won't know if it is really okay unless you stress-test it first. Commented Dec 3, 2015 at 16:18
  • 1
    @MikeNakis Thanks for all your help :) Commented Dec 3, 2015 at 16:19

3 Answers 3

Reset to default
3
+100

"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.
Improve this answer
Sign up to request clarification or add additional context in comments.

8 Comments

Thank you very much for your answer and Tips, they're great :) I'm using Redis + SQL mainly for the Feeds (like in Twitter or FB). App that I'm working on has around 500k users, and each user can follow others, and they get feed based on that. Creation of the feed took me 4+ seconds while I was using SQL only. Now with Redis in play, feed is served in ~500 ms which is huge improvement in speed. And feed is first thing that people see so it must be fast.
And about ToString() + string.Contains: From Redis I get object called ReidsValue which is in my case stringified object, what I did in first place was Deserialization and then I checked for the Id, but then I noticed that string.Contains is a few ms faster. Yeah, this is stupid micro optimization, but some users have more than 10k items in their feeds and when you do operation which is faster even for 1ms over 10.000 times, in the end you save a lot of time. Or am I doing this completely wrong ?
Both 500ms and 4s are extremely slow - I suspect you've got a scalability problem somewhere. If users do have 10k items in their feed, do you actually need to get more than the first (say) 100? How many queries does retrieving a feed cost? Have you looked at indexing, caching, and things like Dapper?
For some sense of perspective, I just did a quick check on an app I maintain: reading 10000 rows of a 4-column table (two ints, a nullable bool and a string that's usually null) takes around 4ms. Reading 10000 rows of a messy, nasty table with 59 columns takes around 45ms. And frankly, I don't think you should need to request 10000 rows to retrieve a feed.
But redis doesn't do joins, so at some point you're denormalizing (flattening) those multiple tables anyhow - and that's likely where the real performance gain is. (I know and have used redis, and if you need it - sure - but it just sounds like you don't really need it here).
|
2

Note: this answer was posted before the OP amended their question to add their code, revealing that this is actually a question of whether async/await is thread-safe.

Static methods are not a problem in and of themselves. If a static method is self-contained and manages to do its job using local variables only, then it is perfectly thread safe.

Problems arise if the static method is not self-contained, (delegates to thread-unsafe code,) or if it manipulates static state in a non-thread safe fashion, i.e. accesses static variables for both read and write outside of a lock() clause.

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

Improve this answer

Comments

1

what you are doing here is synchronizing on a list (deleteTasks). If you do this i would recommend 1 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 a task and await it.

Although i think in this particular case i don't see any issues as soon as you refactor it and DeleteItemFromRedisAsync can get called multiple times in parallel then you will have issues. The reason being is that if multiple threads can modify your list of deleteTasks then you are not longer guaranteed you collect them all (https://msdn.microsoft.com/en-us/library/dd997373(v=vs.110).aspx if 2 threads do an "Add"/Add-to-the-end in a non-thread safe way at the same time then 1 of them is lost) so you might have missed a task when waiting for all of them to finish.

Also i would avoid mixing paradigms. Either use async/await or keep track of a collection of tasks and let methods add to that list. don't do both. This will help the maintainability of your code in the long run. (note, threads can still return a task, you collect those and then wait for all of them. but then the collecting method is responsible for any threading issues instead of it being hidden in the method that is being called)

Improve this answer

1 Comment

Thanks for your answer, so you are saying that if I change DeleteItemFromRedisAsync so it looks like: private static async Task DeleteItemFromRedisAsync(IDatabase redisDb, RedisValue [] feed, string redisKey, string workoutId) and it has await (instead of return): await Task.WhenAll(deleteTasks); - deleteTasks is now initialized inside this function, and not passed as reference I would be good to go? (other code stayed the same, except function call which looks like): await DeleteItemFromRedisAsync

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.