We’re rewarding the question askers & reputations are being recalculated! Read more.

# 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

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

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 ...

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 => { ...

8

Since the only place you should be creating a LockedProxy from is within the call Lockable::GetLockedProxy() you should therefore make the constructor private and friend this function. You don't actually want to give people the ability to make objects of this type themselves. inline LockedProxy(boost::mutex & m, T * obj) :lock(m), t((lock)?obj:...

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 ...

7

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 ...

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

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 ...

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 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 ... 6 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 ... 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 ... 6 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:...

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

I needed something similar, in that I had code that was going to periodically check the database for new records and notify the user with baloon tips. Checking the database every n-secs caused a pause in the UI that I didn't want the user to experience so I moved the procedure to a thread. I had a System.Windows.Forms.Timer tick event, where the Thread was ...

5

No this does not meet the definition of std::lock(). It (std::lock) guarantees that no matter what order you specify the locks in the parameter list you will not fall into a deadlock situation. This basically means that the order you lock the mutex(s) in must be consistent (thus independent of the order they are in the parameter list). Your code is not (m1 ...

5

I read your original question and this one and don't see how using ManualResetEvent/Quartz adds anything valuable. As far as I understand the whole point of sleeping is just to avoid polling the DB too frequently. Here some pseudocode... create table lock (name varchar(50) primary key) bool tryLockNonBlocking(name_to_lock) { try { insert into ...

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

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

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 ...

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