3

I'm currently developping a "Singleton" class, but it has to withstand thread safety as I'm not only accessing, but also settings some stuff.. I was wondering how I could to this - as it seems the locking of stuff with lock(object) feels somewhat hacky when I don't know what it's actually doing and if I have to make every single method thread safe as well?

So far I have this non thread safe code - as in each thread should access the same data. How would I best go about making it thread safe and keep it a singleton?

public class AppSession() {
    private static AppSession _instance = new AppSession();
    public static AppSession Instance { get { return _instance; } }

    private AppSession() { }

    private string _actionName = "none";
    private DateTime? _actionTime = null;        
    public void ActionExecuted(string action) {
        _actionName = action ?? String.Empty;
        _actionTime = DateTime.UtcNow;
    }

    public LastAction {
       get { return $"{_action} at {_actionTime?.Value.ToString()}"; }
    }
}
RecursEve
  • 85
  • 2
  • 7
  • 3
    Rather than using a singleton, consider using dependency injection to manage lifetimes. See also http://stackoverflow.com/questions/137975/what-is-so-bad-about-singletons – TrueWill Feb 26 '17 at 15:06
  • Also try not using a Session if possible. Use caching for performance and not Session. – Filip Cordas Feb 26 '17 at 15:13
  • 3
    Hard to say anything about ActionExecuted because you show only how you write to those fields, not how you read them. – Evk Feb 26 '17 at 15:14
  • thanks, do you mean using an AppSession and inject it like everywhere in every piece of code I use? That seams like a lot of work and very verbose to have each class I have accept a constructor though with that dependency, hence why I went for a singleton in the first place for this scenario :/ The rest of my code is dependency inverted (with help of an IoC container) - this part though I wasn't able to get out without having every class needing it as a dependency (through it's ctor) – RecursEve Feb 26 '17 at 15:14
  • @Evk I'll add that code, basically through a getter with formatting. – RecursEve Feb 26 '17 at 15:15
  • 4
    Without a lock I think it's quite obvious that you might get inconsistency between action and action time in your LastAction getter. One thread might write _acitonName, get interrupted (without updating _actionTime) and then another thread might read LastAction which will be wrong. So if that is full implementation - just add a lock (you can use read-write lock if reads happen much more often than writes). As is - there is no need to use dependency injection for this class, because implementation is very simple and you are not going to replace it with another one (for testing for example). – Evk Feb 26 '17 at 15:29

1 Answers1

5

  1. First I would start by adapting your Singleton to use the Lazy<T> class in the .NET library. "Lazy<T> provides support for lazy initialization" as the MSDN docs state, and it also provides a bool parameter isThreadSafe which when true "makes the instance usable concurrently by mutliple threads".

    private static Lazy<AppSession> _instance = 
new Lazy<AppSession>(
    () => new LazyAppsession(), isThreadSafe: true
);

  2. Secondly I would mark the Lazy<AppSession> as readonly as well, that way no method or code can overwrite the value of the _instance field.

  3. As for the ActionExecuted method, I don't know whether you'd still need to make that one thread safe by locking it... It's something that depends I guess, but don't take my word for it - I hope other can extend my answer on that - as I'd like to know myself as well!

Thus your class would then look something like this - but I'm not sure on the ActionExecuted method, and as I said, the only person I know who can make sense of that would be @JonSkeet.

public class AppSession() {
    private static readonly Lazy<AppSession> _instance = 
        new Lazy<AppSession>(
            () => new LazyAppsession(), isThreadSafe: true
        );
    public static AppSession Instance { 
        get { return _instance.Value; } 
    }

    private AppSession() { }

    private string _actionName = "none";
    private DateTime? _actionTime = null;        
    public void ActionExecuted(string action) {
        _actionName = action ?? String.Empty;
        _actionTime = DateTime.UtcNow;
    }
}
Yves Schelpe
  • 3,024
  • 3
  • 29
  • 59
  • 2
    @JonSkeet would you know whether this is sufficiently protected, especially the `ActionExecuted` method - referring to the fields `_actionName` and `_actionTime`? – Yves Schelpe Feb 26 '17 at 15:04
  • Thanks a lot. Didn't know this (apparantly haha :-)). Are you saying that even with `Lazy` and setting isThreadSafe to `true` the methods withing might still be vulnurable? – RecursEve Feb 26 '17 at 15:06
  • 2
    correct @RecursEve - I'd tweet this question+answer out to the community / @JonSkeet maybe.. Because I honestmy don't know absolutely for sure whether the `ActionExecuted` is sufficiently thread safe now. But at least your `_instance` of the `AppSession` **already is** now due to using `Lazy` with the `isThreadSafe` parameter. – Yves Schelpe Feb 26 '17 at 15:09
  • 2
    You cannot reference people who do not participate in current question, so Jon Skeet does not receive your notifications (just FYI). – Evk Feb 26 '17 at 15:13
  • 3
    It seems like this is basically something that shouldn't be a singleton in the first place, to be honest. Mutable singletons are almost always a bad idea. Note that without any other static members, the simpler form of the singleton without using `Lazy` is fine in almost all cases... the type won't be initialized until it's first needed anyway. (There are some subtleties around static constructors, but I doubt that they're important in this case.) – Jon Skeet Feb 26 '17 at 15:16
  • Thanks everyone. So I might be better off making everything (the field members) static then? The reason I went with Singleton though was because I have to use this _everywhere_ in my codebase, the rest of the code is nicely depencey "inverted" - but I just didn't like injecting this in every constructor of having it as a property reference everywhere? – RecursEve Feb 26 '17 at 15:19
  • @RecursEve - so it's a piece of code you're using in 80% of the codebase to track things? I didn't think of it not having to be a Singleton, but I understand the rest of the people suggesting not to use it indeed. – Yves Schelpe Feb 26 '17 at 15:22
  • @RecursEve - I honestly don't know that, if you're using it like a lot in your codebase, it seems like a sort of logger dependency - and there are various opinions out there on how tightly coupled you make those :)... Your own decision I guess.. – Yves Schelpe Feb 26 '17 at 15:26