0

I have a minutely scheduled task and I only want to be accessed 1 thread at a time, I use SemaphoreSlim as C# code below, but the other thread can enter before the previous thread has been completely finished.. what I missed?

public static async Task<bool> Update_ThumbnailsAsync(long _survey_pid, bool _thumbnails)
{
    using (var _sem = new SemaphoreSlim(1, 1))
    {
        await _sem.WaitAsync();

        ... doing loop process here
        ...
        ...
        ...
     }
 }
DevDon
  • 101
  • 7
  • 1
    You're keeping the ```SemaphoreSlim``` in the local scope. That needs to be global to work. – devsmn Jul 16 '20 at 09:04
  • Why are you using semaphores with tasks? Tasks are *not* threads. What are you trying to do? There are better ways to achieve this, whatever it is. If you want to control how many workers can do a job at a time you can use an `ActionBlock` or a Channel with a single consumer – Panagiotis Kanavos Jul 16 '20 at 09:04
  • @PanagiotisKanavos I very often find myself needing a SemaphoreSlim in single-threaded async code. The reason is that `async` methods can be re-entrant even on a single thread: although it's only possible for the thread to be executing a single line of code in the method at once, it is possible for multiple parts of the method to be executing at once (although all, or all but one part will be stuck on `await`s). Sometimes a method will do [update state, await, update state], and you need to ensure that nothing else calls into the same method and modifies that state during the await – canton7 Jul 16 '20 at 09:16
  • 2
    @PanagiotisKanavos, what is a problem with tasks and `SemaphoreSlim`? Can you add a reference to read more about this topic? I am not using data flow and have no plans to start using it any time soon. – Sinatr Jul 16 '20 at 09:16

1 Answers1

5

You are declaring and instantiating the semaphore inside your function, and subsequent calls to the function will instantiate their own semaphores. You need to move your semaphore outside the scope of your function.

something like this:

private static _sem = new SemaphoreSlim(1, 1);
public static async Task<bool> Update_ThumbnailsAsync(long _survey_pid, bool _thumbnails)
{   
    await _sem.WaitAsync();

    ... doing loop process here
    ...
    ...
    ...
 }
Lev
  • 583
  • 3
  • 15
  • 4
    You forgot `static` and optionally: `readonly` and disposing `IDisposable`. – Sinatr Jul 16 '20 at 09:11
  • if I have 3 methods then do I need to have _sem1, _sem2 and _sem3 to control for each async methods? – DevDon Jul 16 '20 at 09:15
  • @DevDon Yes, that means each method's 'loop process' can only be executed one thread at a time. – Lev Jul 16 '20 at 09:21
  • @Sinatr if it's static, and readonly then it doesn't need to be disposed. It's either readonly OR disposing IDisposable when it's replaced by a new Semaphore – Lev Jul 16 '20 at 21:58
  • @Lev, you can call methods of instance contained in [readonly](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/readonly) field, including `Dispose()` method. [SemaphoreSlim](https://learn.microsoft.com/en-us/dotnet/api/system.threading.semaphoreslim) must be [disposed](https://stackoverflow.com/q/32033416/1997232). My *"optionally"* means that you don't have to add disposing to your answer. – Sinatr Jul 17 '20 at 07:14
  • @Sinatr My point is that `static readonly SemaphoeSlim` field will have the lifetime of the application, it will never be garbage collected and it will never be finalized and it will never be disposed. You will want to dispose it it's not 'readonly' and you replace it with another instance of `SemaphoeSlim`, then you do need to dispose of the previous instance. – Lev Jul 17 '20 at 12:42
  • [Static disposable objects](https://stackoverflow.com/a/12209741/1379655) – Lev Jul 17 '20 at 12:54
  • I am not talking about application life-time memory leaks, where you create new instances and may run out of memory, I am talking about [gdi leaks](https://www.google.com/search?q=c%23+gdi+leaks+site:stackoverflow.com). If `SemaphoreSlim` uses gdi resources - it **must** be disposed. You shouldn't rely on finalizers (actually [I don't see](https://referencesource.microsoft.com/#mscorlib/system/threading/SemaphoreSlim.cs,48) one in source). [This answer](https://stackoverflow.com/a/32033463/1997232) has to be wrong for you to be right. – Sinatr Jul 17 '20 at 13:46
  • @Sinatr that answer is perfectly fine and correct when talking about ordinary types, but this is a static type. Ordinary types, implementing IDisposable, must be disposed before going out of scope. A static type will go out of scope when the application shuts down, it is safe to assume that the process ends, when process ends all GDI handles will be destroyed with the process, so there is really no point in releasing GDI handles right before they are destroyed. – Lev Jul 18 '20 at 23:01
  • @Lev, I'd be really grateful for a link to the statement *"all gdi handles will be destroyed upon process exit"*. It contradict to my experience, but after a quick googling I can't confirm my position either, which is "you must release all gdi objects yourself before exiting". – Sinatr Jul 20 '20 at 07:16
  • @Sinatr [Is leaked memory freed up when the program exits?](https://stackoverflow.com/questions/2975831/is-leaked-memory-freed-up-when-the-program-exits) *The OS still keeps track of all the memory allocated to a process, and will free it when that process terminates.* [GDI Objects](https://learn.microsoft.com/en-us/windows/win32/sysinfo/gdi-objects#managing-gdi-objects) *GDI objects support only one handle per object. Handles to GDI objects are private to a process. That is, only the process that created the GDI object can use the object handle.* – Lev Jul 20 '20 at 07:49
  • @Sinatr That's a good point, I haven't found a source where this is stated explicitly, and assumed that this is the case. Inferring from the 2 sources in previous comment, it is logical to assume that the GDI objects are unallocated. I may be wrong, I will try to test it – Lev Jul 20 '20 at 07:54