# Tag Info

16

First of all, your locking seems ok, I don't expect any drawbacks by using multiple threads. That being said, let us review this from bottom to top. public enum Tryb { FILE = 0, CONSOLE = 1, FILEANDCONSOLE = 2 } The name of that enum is at the nicest say unclear. A name like LogTarget would be more clear and its meaning would be obvious ...

11

The obvious problem with this is it will detect any function that uses TranLock calling any other function that also uses TranLock (since there is only a single global count). That is, if func() (which uses TranLock) calls bar() (which also uses TranLock) then bar() will erroneously fail. In order to make a lock like this that can be used with multiple ...

11

I don't see anything in this code that needs to be commented. To echo what other people said, comments should clarify code and they should never say what the code already says. I would recommend commenting code under these conditions: You can't tell what the code is doing by reading it You encountered a wonky bug that took you a long time to figure out, ...

11

Commenting the method At the minimum, public API methods should always be commented. The posted code is an implementation. You should have the method declaration, serving as the interface definition, somewhere else. It's best when the actual implementation is not even visible to users of your API. That's good information hiding. So, where this method is ...

11

Your code looks sensible, with two exceptions, the unlock should only notify when the lock is unlocked.... your code currently allows for asymmetrical notifications (excessive notifications). Consider: public synchronized void unlock(){ if(Thread.currentThread() == lockedBy){ lockCount--; } if(lockCount == 0){ isLocked = false; ...

10

Besides the unjustified use of goto, I also object to the use of the number 4 for an operation that you try up to 6 times. Counting loops are usually best written as for loops. public function wait_lock() { for ($tries = 0;$tries < 6 && !($lock =$this->is_enabled('cache-lock')); ++$tries) { sleep(1); } if (!$lock) { ...

9

Instead of checking the time from each thread on each iteration, I would use a Timer: int count = 0; TimeSpan reportPeriod = TimeSpan.FromMinutes(0.1); using (new Timer( _ => Console.WriteLine("{0}: Extracted {1} urls", DateTime.Now, count), null, reportPeriod, reportPeriod)) { Parallel.ForEach( list, tuple => { ...

9

I don't see memory leaks here, as well as cases where lock is not applied. From design point of view I improve it a little bit. Currently you require customers to get the item lock and then lock on the Lock property. You can simplify it by locking right away, and unlocking in within Dispose method: public class ItemLock : IDisposable { private static ...

8

What I don't like about your approach is that the lock objects are never removed from FooServiceCacheLocks, even when the object is removed from the cache. One way to simplify your code would be to combine MemoryCache with Lazy: instances of Lazy are cheap (as long as you don't access its Value) so you can create more of them than needed. With that your ...

8

This code block String strAppDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().GetName().CodeBase); strAppDir = strAppDir.Replace("file:\\", ""); // dla windowsa strAppDir = strAppDir.Replace("file:", ""); // dla linuksa string file = Path.Combine(strAppDir, "error.txt"); is clearly copy-pasted. :) You should extract this logic to separate ...

8

According to information you provided it's not necessary to wrap it with lock. Let's consider situation you described. You wrote: There will be multiple threads instantiating this class So there will be one instance for each thread. There's no synchronisation needed, because only one thread will be accessing your class at a time. Other scenarios: Single ...

7

The only weak point is that the implementation is not copy safe nor there exist mechanism for ensuring copy protection. I would hide its underlying type and return as sync.Locker, so it can't be mis-used: type spinLock uint32 func (sl *spinLock) Lock() { for !atomic.CompareAndSwapUint32((*uint32)(sl), 0, 1) { runtime.Gosched() //without this it ...

7

Like @Gareth mentioned in his comment it would be perfect to use the ConcurrentQueue<T>. You answered @Gareth the queue needs to be bounded in size but where is the problem. We can just add a check to the Add() method and if the queue's size is equal to the bounded size we will just dequeue one item. If the passed in size parameter of the ...

7

A lock works here, and it's functional, but it does come with some drawbacks in this case. If you and I both submit the request at the same time, one of them will be delayed even though there wasn't actually a reason. Now you've introduced a potentially hazardous situation, what happens if a lot of distinct persons submit requests at once? The best option ...

7

General impressions - nice neat code, well commented. It's good that you use trap to clean up on exit. Shellcheck picks up a couple of oversights: 206596.sh:11:11: note: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. [SC2181] 206596.sh:22:1: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164] 206596.sh:... 6 You should put the lock statements before the try{}finally. For example if the thread is interrupted before your interruptible acquire then lockInterruptibly will throw and the finally will try to unlock a non-locked lock (which will throw a IllegalMonitorStateException). There is no need for the signalAll in the acquire methods, as nothing happens that ... 6 There are a number of issues I see here related to the implementation. try -> lock vs. lock -> try ratchet freak has already pointed out that the lock.lock() should be outside the try: mLock.lock(); try { .... do work here .... } finally { mLock.unlock(); } This needs to be said multiple times, so I am saying it again. TODO All those TODO ... 6 I would define domain specific error classes like so: module Lockable Error = Class.new StandardError AlreadyLocked = Class.new Error AlreadyUnlocked = Class.new Error def lock! raise AlreadyLocked if locked? @locked = true end def unlock! raise AlreadyUnlocked unless locked? @locked = false end def locked? ... 6 Why not just say public function wait_lock() {$try_count = 0; while ( ! $this->is_enabled('cache-lock') ) {$try_count++; if ( $try_count >= 6 ) { // the following method is a short cut for throwing an exception.$this->e('cache error. wait a few moments and try again. if the problem persists, ...

6

What happens if this is called by something that hasn't captured the object? public void Release() { lock (_lock) IsCaptured = false; } Honestly, I don't see what this is giving you that you don't get from: Monitor.TryEnter(lockObj) and Monitor.Exit(lockObj); Unless you want to release the capture from a different thread, in which case it ...

6

Spinlocks should only be held for a very short time Spinlocks are expensive; they cause a thread that tries to take a lock to use 100% CPU time. So when you have the lock, you should release it as quickly as possible. When you call enterAsReader(), you do not release the lock. You indicated yourself that the work you do as a writer might be manipulating "...

6

First, I suppose it would be cheating to just use a std::shared_mutex, right? :) Second, it seems a little weird to declare uintmax_t writers for a variable that can only ever hold the value 0 or 1. But, on the other hand, the symmetry between readers and writers is kind of nice. uintmax_t readers = {0}, writers = {0}; Strongly prefer to declare one ...

5

A few notes to address your specific questions: Will this perform as expected? Only you can answer that after testing it thoroughly. Does sessions need to be a ConcurrentDictionary or does a regular Dictionary work fine here since I'm locking any access of the one key. As the lock keys are different for different sessions, it is a whole lot more safe ...

5

In this answer I explain in details why one should not throw System.Exception. You should be throwing InvalidOperationException in the case of the astronomically unlikely situation, and probably an ArgumentException in the case of the non-existing session. As far as thread safety is concerned, I don't write multithreaded code very often so I might be ...

5

If you swap pointers to the mutexes after a failed iteration, you can get better performance than either your original code, or the ordered solution shown in the currently accepted answer. void lock(pthread_mutex_t* m1, pthread_mutex_t* m2) { while(1) { pthread_mutex_lock(m1); if(pthread_mutex_trylock(m2) == 0) { // if lock succesfull break; ...

5

Herb suggested another way for associating mutexes with data in c++11 more recently than the linked article in this video. The whole video is worth watching in my opinion, but he explains a wrapper pattern around minute 38 and shows how it applies to associating a mutex in minute 40. His implementation looks like this: template< class T > class ...

5

Usually when I comment I look over the code and see what might be unclear to other people reading the code, and then I try to explain that line in plain english. What's clear to you about your own code isn't always as clear to others. So a good tip would also be to have others look over your code, ask them what they don't understand about the code and then ...

5

Formatting Your indentation is fine and consistent. However, I strongly discourage you from omitting the optional braces with for, if, and else. Every time you do so, you are contributing to a future coding accident. (Don't think that it can't happen to you!) You may wish to consider indenting the block of code in the critical section, so that the ...

5

This is largely a duplicate of https://stackoverflow.com/questions/6935442/x86-spinlock-using-cmpxchg, https://stackoverflow.com/questions/11959374/fastest-inline-assembly-spinlock, etc. (1) Your code works only on 32-bit x86. If you're using 64-bit (x86-64), the calling convention is totally wrong (you need to save and restore %rbp rather than %ebp, and ...

5

We can simplify that a lot. You are not really using RAII on the CriticalSection as you declare it in one place then initialize it in another. But CRITICAL_SECTION is a type as you pass a pointer around. Why not simplify a bit: class GetLock; class CriticalSection { public: CriticalSection() { InitializeCriticalSection(&m_CS); // get a ...

Only top voted, non community-wiki answers of a minimum length are eligible