Makes core.sync.mutex.Mutex usable in modern code by PetarKirov · Pull Request #1728 · dlang/druntime

PetarKirov · 2016-12-28T14:52:25Z

Makes core.sync.mutex.Mutex usable in modern @safe nothrow @nogc D code:

Make Mutex.this() / this(Object) / ~this() nothrow and @nogc
Add shared overloads to Mutex.this, Mutex.lock, Mutex.unlock and Mutex.tryLock
Add tryLock_nothrow variant of Mutex.tryLock - for use in nothrow @nogc code, similar to Mutex.lock_nothrow and Mutex.unlock_nothrow.

TODO: Add unittests.

Please review commit by commit, as it would be more clear why each change is made.

jmdavis

Aside from the comments, I think that this looks good. Long term, I would think that we'd want to force shared on the whole API and deprecate the thread-local functions, but it's better to wait at least a release before doing that so that it's possible for folks to build their code for both the last release and master without getting a bunch of deprecation warnings that they can't fix.

jmdavis · 2017-01-13T05:31:09Z

src/core/sync/mutex.d

-                throw new SyncError( "Unable to initialize mutex" );
-            scope(exit) pthread_mutexattr_destroy( &attr );
+            !pthread_mutexattr_init(&attr) ||
+                assert (0, "Unable to initialize mutex");


core.internal.abort would be closer to the previous functionality, since it would still print out in release mode. And given how incredibly confusing this failure would be if it were to ever occur without an error message, it would probably be better to use abort in all the cases in this commit where you use assert(0). And it'll work with @nogc.

jmdavis · 2017-01-13T05:40:28Z

src/core/sys/posix/pthread.d

 int pthread_mutex_destroy(pthread_mutex_t*);
 int pthread_mutex_init(pthread_mutex_t*, pthread_mutexattr_t*) @trusted;
 int pthread_mutex_lock(pthread_mutex_t*);
+int pthread_mutex_lock(shared(pthread_mutex_t)*);


Wouldn't it make more sense to just leave the C functions as-is and cast away shared when calling them? After all, shared does not exist in C. I'm surprised that this even works. It's also pretty weird to have shared on only some of them when they're all clearly in the same boat.

I deliberately made that change because it bothered me when porting C and C++ code to D, which was directly using the platform specific APIs. Especially when porting a C project where there were absolutely zero abstractions and I needed to add casts to almost every function, since almost every function used pthread_mutex directly and I wanted to use shared instead of __gshared.

I'm surprised that this even works

You shouldn't be, if you consider that strchr works the same way in C++. In C all symbols (including functions) are mangled the same as their name (give or take an underscore at the beginning). That allows (sometimes correctly and sometimes not) to map multiple overloads from C++ or D to the same C function. (Note: some C++ compilers allow C function overloading but require vendor specific extensions similar to D's pragma (mangle).)

Also consider that in D you can overload extern (C) functions on attributes like nothrow. Especially useful for function pointer parameters.

It's also pretty weird to have shared on only some of them when they're all clearly in the same boat.

Not all of them can be called concurrently on the same object. For example pthread_mutex_destroy is not thread-safe - it should be called only when the (conceptual) ref count of the mutex drops to 1. Also removing the non-shared overloads would be breaking change.

Functions like pthread_rwlock_*lock should also have shared overloads, but I prefer to leave this for another day.

yah shared is good here

BTW: LDC does not allow multiple definitions that mangle to the same name, so this will probably come up again when updating druntime in LDC. Not sure whether that restriction should be lifted in LDC, though.

Oh yeah, if I remember correctly that was changed recently. The correct way to go about that in LDC, in my opinion, is to allow overloading if and only if all overloads have the same ABI as should be the case here.

To be pedantically correct, those functions should have been declared as taking shared parameters in the first place, but leaving only the shared versions would be a breaking change now.

If that's the approach to take, then pretty much every C/C++ function should have shared parameters, because in reality, that's what they are. We just usually ignore that fact, because it usually doesn't matter.

Just like const prevents accidental mutation, shared guards against accidental use of non-thread-safe functions on shared data. Only functions that are proven to operate in a thread-safe way on shared data should be marked as shared.
In general, when we don't know whether a function is thread-safe we should not call it on shared data without sufficient synchronization and therefore we should not mark any of its parameters as shared.

There is zero guarantee that operating on something that's shared is thread-safe. If anything, it's the opposite. shared means that the data is not thread-local. So, proper synchronization is required. To be perfectly correct, basically all extern(C) functions should be have shared parameters and return types, because variables in C are not thread-local unless you jump through a bunch of hoops, using stuff that's not actually part of the language.

As it stands, we don't bother marking types on extern(C) functions with shared, and it's up to the programmer to understand what they're getting into and making sure that that they don't violate the thread-local guarantees that the D compiler provides for D code. If we then decide to start marking extern(C) functions with shared, the ones that most need to be marked are the ones that aren't thread-safe, not the ones that are.

Deciding to use shared on an extern(C) function because it's thread-safe, makes no sense at all. If it's thread-safe, then you can get way with passing thread-local data, whereas if it's not, then the data needs to be treated as shared by the D code - whether that's by marking up the extern(C) function with shared or by having the D code just be aware of the semantics and and dealing with them accordingly.

So, if we want to start marking extern(C) functions with shared, then we should be marking the ones that clearly operate on data that is intended to be shared across threads as shared, and whether the function is thread-safe is pretty much irrelevant.

jmdavis · 2017-01-13T05:56:56Z

src/core/sync/mutex.d

@@ -192,7 +230,20 @@ class Mutex :
     * Returns:


You need to fix it so that tryLock's documentation no longer says that it throws a SyncError. And actually, that bit of documentation is wrong even without your changes, since it just calls a C function and doesn't throw anything.

Thanks, fixed.

jmdavis · 2017-01-13T06:04:05Z

src/core/sync/mutex.d


-    // undocumented function for internal use
+    /// ditto
    final void lock_nothrow(this Q)() nothrow @trusted @nogc


Are you documenting these because the normal versions can't be made nothrow and @nogc without breaking code? You can overload on attributes, but that still breaks code since you get an error about overriding one of the functions shadowing the others. I can see someone overriding lock and unlock for debugging purposes, but it seems really dumb for the version of the function that you would ideally call to not be lock or unlock... Also, the _nothrow function names don't actually follow the official naming convention, though lockNothrow does seem a bit ugly. As such, I'd be inclined to find a better name for them, but I don't know what it would be. Something like lockMutex would follow the naming convention and doesn't seem as ugly, but it seems redundant, and it doesn't make the difference between it and lock obvious.

Since those methods (lock, unlock, tryLock) are virtual, we can't add attributes to them without breaking code. @MartinNowak attempted to enforce nothrow back then when 2.067 was to be released, but that broke vibe.d and had to be reverted. You can read more details about why vibe.d needed to throw from the locking primitives in this thread (in short - in order to implement interruptable tasks with proper resource cleanup).

I'm not 100% sure if we should document those functions, but that's what we should actually try to advertise as it is more friendly to code that tries to apply attributes where possible. I'm also not fan of the names, but considering that they're already public (albeit undocumented) I think we're kind of stuck with them. The only alternative names that I can think of are acquire and release, though tryAcquire sound bad to me.

Perhaps we can add lockExcept / unlockExcept virtual functions and deprecate the throwing methods, but I prefer not to do that in this change.

PetarKirov · 2017-01-14T00:52:22Z

Thanks for the review @jmdavis. I've added unittests and I've addressed all your points.
Let me know if there's anything left to do.

PetarKirov · 2017-01-14T10:14:59Z

Ping @andralex @WalterBright can you help me move forward with this PR? I need it continue working on adding more attributes in druntime and phobos.

andralex · 2017-01-14T12:46:25Z

ping @jmdavis

andralex · 2017-01-14T12:51:00Z

Nice work!

andralex · 2017-01-14T12:46:46Z

src/core/sync/mutex.d

 public import core.sync.exception;

-version( Windows )
+version (Windows)


andralex · 2017-01-14T12:49:20Z

src/core/sync/mutex.d

+    }
+
+    /// ditto
+    final void unlock_nothrow(this Q)() nothrow @trusted @nogc


unlockNothrow? (though truth be told the underscore convention is also used)

unlock_nothrow was introduced in 2.067, so there's not much that I can do now.

andralex · 2017-01-14T12:50:47Z

src/core/sys/posix/pthread.d

 int pthread_mutex_destroy(pthread_mutex_t*);
 int pthread_mutex_init(pthread_mutex_t*, pthread_mutexattr_t*) @trusted;
 int pthread_mutex_lock(pthread_mutex_t*);
+int pthread_mutex_lock(shared(pthread_mutex_t)*);


yah shared is good here

andralex · 2017-01-14T12:51:40Z

OK let's move this forward once @jmdavis takes a second look

PetarKirov · 2017-01-14T13:13:25Z

Thanks for the review @andralex!

PetarKirov · 2017-01-18T10:10:54Z

Ping @jmdavis is there anything left for me to do?

jmdavis

I think that the one bit with pthread_mutexattr_destroy should be fixed (which I missed on my previous review), and then it's ready to go. I'm not entirely enthused with the idea of marking some of the pthread functions with shared, but Andrei seems to be in favor, so it's okay, I guess.

jmdavis · 2017-01-24T10:12:15Z

src/core/sync/mutex.d


-            if( pthread_mutexattr_settype( &attr, PTHREAD_MUTEX_RECURSIVE ) )
-                throw new SyncError( "Unable to initialize mutex" );
+            scope (exit) pthread_mutexattr_destroy(&attr);


It seems inconsistent that the result of pthread_mutexattr_destroy is not checked. It probably should be. I'm not sure that that's really blocker though, since it hasn't been being checked. It would just be an improvement that was missed with this PR.

Sure, I'll fix this later today.

Fixed & rebased

Additionally add `shared` overloads to: + core.sys.windows.winbase: * EnterCriticalSection * LeaveCriticalSection * TryEnterCriticalSection + core.sys.posix.pthread: * pthread_mutex_lock * pthread_mutex_unlock * pthread_mutex_trylock Also add `tryLock_nothrow` version of `Mutex.tryLock`.

…messages

* Also make the error messages more descriptive

PetarKirov · 2017-01-25T08:30:39Z

@jmdavis Merge pls?

Even though I added three unittests codecov is complaining because the aborts are not tested and to be honest, I have no intention of doing so, because that's code that shouldn't really be executed.

andralex · 2017-01-25T12:31:34Z

src/core/sync/mutex.d

 /**
 * This class represents a general purpose, recursive mutex.
+ *
+ * Implemented using pthread_mutex on Posix and CRITICAL_SECTION


Wrap pthread_mutex and CRITICAL_SECTION in backquotes

andralex · 2017-01-25T12:32:20Z

src/core/sync/mutex.d

+        version (Windows)
        {
-            InitializeCriticalSection( &m_hndl );
+            InitializeCriticalSection(cast(CRITICAL_SECTION*)&m_hndl);


space after cast(...) cc @wilzbach

andralex · 2017-01-25T12:33:10Z

src/core/sync/mutex.d

-                throw new SyncError( "Unable to initialize mutex" );
-            scope(exit) pthread_mutexattr_destroy( &attr );
+            !pthread_mutexattr_init(&attr) ||
+                abort("Error: pthread_mutexattr_init failed!");


No exclamation marks please, just use a period.

andralex · 2017-01-25T12:33:17Z

src/core/sync/mutex.d

-            if( pthread_mutexattr_settype( &attr, PTHREAD_MUTEX_RECURSIVE ) )
-                throw new SyncError( "Unable to initialize mutex" );
+            scope (exit) !pthread_mutexattr_destroy(&attr) ||
+                abort("Error: pthread_mutexattr_destroy failed!");


andralex · 2017-01-25T12:33:22Z

src/core/sync/mutex.d

-            if( pthread_mutex_init( &m_hndl, &attr ) )
-                throw new SyncError( "Unable to initialize mutex" );
+            !pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE) ||
+                abort("Error: pthread_mutexattr_settype failed!");


andralex · 2017-01-25T12:33:28Z

src/core/sync/mutex.d

+                abort("Error: pthread_mutexattr_settype failed!");
+
+            !pthread_mutex_init(cast(pthread_mutex_t*)&m_hndl, &attr) ||
+                abort("Error: pthread_mutex_init failed!");


andralex · 2017-01-25T12:33:44Z

src/core/sync/mutex.d

+
        m_proxy.link = this;
-        this.__monitor = &m_proxy;
+        this.__monitor = cast(void*)&m_proxy;


andralex · 2017-01-25T12:34:12Z

src/core/sync/mutex.d

     *  o must not already have a monitor.
     */
-    this( Object o ) nothrow @trusted
+    this(Object o) @trusted nothrow @nogc


o is awkward

What about obj? Any better suggestions?

obj is fine - pretty much anything but o, O, I, and l :)

andralex · 2017-01-25T12:34:45Z

src/core/sync/mutex.d

-            assert( !rc, "Unable to destroy mutex" );
+            import core.internal.abort : abort;
+            !pthread_mutex_destroy(&m_hndl) ||
+                abort("Unable to destroy mutex");


period at end

andralex · 2017-01-25T12:38:36Z

To be perfectly correct, basically all extern(C) functions should be have shared parameters and return types

That's incorrect. Most extern(C) functions assume they are not called with racy unsynchronized parameters, and the fact that we can enforce that in D at the type system level is a plus, not a minus.

PetarKirov · 2017-01-25T15:23:12Z

@andralex fixed & rebased.

jmdavis · 2017-01-25T18:41:18Z

To be perfectly correct, basically all extern(C) functions should be have shared parameters and return types

That's incorrect. Most extern(C) functions assume they are not called with racy unsynchronized parameters, and the fact that we can enforce that in D at the type system level is a plus, not a minus.

Yes and no. The C stuff isn't thread-local, and that means that technically, it's all shared, even if it's not marked as such. In effect, it's all __gshared. It's really shared, but we're lying about it so that it works with D code. So, if we were to mark it all up as shared, that would be completely correct, whereas marking them all as thread-local is a lie. It's just that it would be super annoying to use them if they were all marked with shared, and most_C functions can be called safely as if they were thread-local. So, the approach that we've been taking of not using shared on any of them works without being super annoying so long as the programmer is aware enough of what's going on to treat the functions that really aren't thread safe as if they were shared.

Adjusting that approach so that we mark extern(C) functions with shared when we know that they aren't thread-safe then makes it safer for the programmer, because they're then actually forced to deal with shared when we know that it matters. They still have to be careful with other extern(C) functions that aren't marked with shared and be aware of what those functions actually do, because it's quite possible that they aren't thread-safe, since they are actually operating on shared data, but marking the ones that are definitely not thread-safe as shared at least makes it clear for those functions and removes the need for the programmer to figure it out in their case. So, that approach does make sense. But it's still true that all of the other extern(C) functions are operating on shared data that's not properly marked as shared. We're just letting it slide, because the code is theoretically thread-safe in spite of that.

So, the most correct thing would be to marked the all as shared. It just wouldn't be the safest thing to do, because then everyone would just be casting left and right, whereas if we use shared selectively, they'll pay attention when it's there. Right now, since we're not using shared with extern(C), we're basically doing the equivalent of slapping @trusted on every extern(C) function except that it's for thread-safety. Selectively using shared on extern(C) functions is then then not slapping the @trusted of thread-safety on those functions. But thanks to how the type system treats extern(C) functions, by default, they're assumed to be thread-safe when they really should be assumed to not be thread-safe. Selectively using shared therefore makes sense, but it's like we're having to mark stuff with @system manually rather than the compiler catching it for us. Unfortunately, fixing that would break far to much code, so selectively using shared is probably the best that we can do at this point.

andralex · 2017-01-25T18:59:54Z

Yes and no. The C stuff isn't thread-local, and that means that technically, it's all shared, even if it's not marked as such. In effect, it's all __gshared. It's really shared, but we're lying about it so that it works with D code.

This is wrong, almost backwards actually. The lengthy explanation that follows does not help either :). Consider printf(const char* format, ...); Can a shared format string be passed by C code into C code? Absolutely. Does printf implement any mechanism for working with format strings that are actually shared across threads? No. Would printf actually work well if the string were modified concurrently by another thread? Most assuredly not. It follows that the printf signature is not sufficient - there is an assumption embedded so deeply it's not even part of the documentation that the format string is supposed to be effectively thread-local for the duration of the call. (This is of course not the case for all C functions but for a good fraction thereof.) So the D signature of printf is paradoxically more precise because it clarifies the assumption that the format string (and all other parameters) are thread-local.

jmdavis · 2017-01-25T20:03:42Z

The issue is that unlike D code, the C code makes no guarantees about operating on thread-local data, because normally, everything in C is shared. So, any time that D is calling a C function, the programmer has to be aware of whether the function is actually thread-safe or not and program accordingly rather than having that be clear in the type-system like it is with D. So, if a programmer is sure that the extern(C) function is thread-safe, then not having it be marked as shared is perfectly fine, and then the type system _does _help them - but it's up to the programmer to figure that out and mark the function accordingly. printf can obviously be treated as thread-safe, but because the compiler doesn't verify thread-safety anywhere with extern(C) functions, somewhere in the process a D programmer needs to verify it.

Basically, because extern(C) provides no compiler guarantees about what's going on inside these functions, every time the programmer is not using shared on an extern(C) function, they're either saying that they're not worrying about shared and it's up to the caller to deal with it correctly (which, for better or worse, is what we've done historically), or they're promising that it's actually thread-safe. On the other hand, if they're marking it as shared, then they're saying that they think that it's not thread-safe. The safest default would be to just use shared for all extern(C) functions precisely for the reason that assuming that a function is @system unless it's been verified by the compiler or programmer to be otherwise makes sense. But instead, it's basically entirely up to the programmer to mark the function correctly, and no one uses shared on extern(C) functions. So, the verification is always left up to the caller, and there's no way that that's always done correctly.

So, moving to marking extern(C) functions with shared where they're not thread-safe would then at least remove the need for the caller to do that verification, but we're then left with the problem that the assumption is that an extern(C) function without shared is thread-safe when the reality of the matter is that it could simply be that the person who did the bindings didn't bother with shared (and basically no one has thus far - certainly not in druntime). Still, marking extern(C) functions with shared when we know that they're not thread-safe is likely the best course of action.

I still think that it would be safer though if the language treated extern(C) functions as implicitly shared and forced the programmer to mark them as thread-local, even if most C functions would then be marked that way, since the way it is easily leads to passing thread-local D stuff to C code that isn't thread-safe. But then we'd need a special attribute just for extern(C) functions, because that's the complete opposite of what the D code does, and unfortunately, human nature being what it is, programmers would probably just slap the @local or whatever on extern(C) left and right rather than actually verifying that the function is thread-safe, particularly since most C functions are in spite of the fact that C itself doesn't do anything with thread-local or guaranteeing thread-safety.

So, I guess we disagree on what the default really should be with regards to shared and extern(C), but at this point, I don't think that we disagree about when an extern(C) function should be marked as shared. It is effectively a change in policy in how we handle C bindings though, and all of the druntime C bindings should probably be examined and change accordingly (at minimum with a shared overload being added, and ideally, at some point, the thread-local overloads would be deprecated).

andralex · 2017-01-25T20:28:34Z

The issue is that unlike D code, the C code makes no guarantees about operating on thread-local data, because normally, everything in C is shared.

No. Normally most in C is NOT shared, it's just that that information is not represented in the type system. Typical C programs have carefully-managed portions of data that is actually shared and specific narrow APIs for it. Don't confuse de jure with de facto.

So, any time that D is calling a C function, the programmer has to be aware of whether the function is actually thread-safe or not and program accordingly rather than having that be clear in the type-system like it is with D.

Not if the D signature of the C binding is written correctly.

So, if a programmer is sure that the extern(C) function is thread-safe, then not having it be marked as shared is perfectly fine, and then the type system _does _help them - but it's up to the programmer to figure that out and mark the function accordingly.

Most definitely @shared is the wrong default to go by seeing as the vast majority of C functions already assume the data is not shared.

printf can obviously be treated as thread-safe, but because the compiler doesn't verify thread-safety anywhere with extern(C) functions, somewhere in the process a D programmer needs to verify it.

The onus is small - only for the handful of C functions that actually do perform synchronization. The situation doesn't warrant multiple walls of texts.

dnadlinger · 2017-01-25T20:29:14Z

On the other hand, if they're marking it as shared, then they're saying that they think that it's not thread-safe.

That's completely backwards when it comes to the interface of a function. However ill-defined shared is currently, on a function parameter it means that the function is able to deal with shared data. Pretty much only synchronization primitives and operations on lock free data structures fall into this category, but the vast majority of parameters doesn't – as Andrei points out, printf certainly can't be expected to handle concurrent modifications to the format string.

andralex · 2017-01-26T14:19:08Z

Auto-merge toggled on

PetarKirov · 2017-01-26T14:56:55Z

Thanks, @andralex! Can you also have a look at #1726?

MartinNowak · 2017-01-30T16:48:26Z

src/core/sync/mutex.d

+    }
+
+    /// ditto
+    this() shared @trusted nothrow @nogc


@ZombineDev
Unfortunately this broke sub-classes, see Issue 17130 – [Reg 2.074] ambiguous implicit super call when inheriting core.sync.mutex.Mutex. Would be nicer to fix this in dmd though.

This indeed looks like a compiler bug. If the base class has multiple default constructors, the compiler should synthesize multiple default constructors in the derived class. I.e. the base class and the derived class should have equivalent default constructor overload sets. (Assuming no user-defined default constructors in the derived class.)

Do you think that the front-end can be fixed in a relatively short-term period? If not, probably the best course of action would be to revert this PR, though I prefer not to.

Hopefully the project tester can prevent such issues from coming up next time. I wonder why it didn't run for the whole month my PR was up for review.

PetarKirov changed the title ~~Add attributes to core sync mutex~~ Makes core.sync.mutex.Mutex usable in modern @safe nothrow @nogc code Dec 28, 2016

PetarKirov changed the title ~~Makes core.sync.mutex.Mutex usable in modern @safe nothrow @nogc code~~ Makes core.sync.mutex.Mutex usable in modern code Dec 28, 2016

This was referenced Dec 28, 2016

Make AssertError a singleton #1710

Closed

Add attributes to core.thread #1726

Merged

PetarKirov force-pushed the add-attributes-to-core-sync-mutex branch 3 times, most recently from b403473 to fb291b4 Compare December 28, 2016 21:22

jmdavis suggested changes Jan 13, 2017

View reviewed changes

PetarKirov force-pushed the add-attributes-to-core-sync-mutex branch 2 times, most recently from b0fe0f1 to 2ceb5cc Compare January 14, 2017 00:51

andralex approved these changes Jan 14, 2017

View reviewed changes

PetarKirov mentioned this pull request Jan 18, 2017

Add pure wrappers for C style allocation #1746

Merged

jmdavis suggested changes Jan 24, 2017

View reviewed changes

PetarKirov added 10 commits January 24, 2017 22:07

Fix coding style in core.sync.mutex

9f21c42

Make core.sync.mutex.Mutex.this()/this(Object)/~this() nothrow and @nogc

5523188

[WIP] Improve core.sync.mutex documentation

f4810b4

[core.sync.mutex] Add a note to the docs about nothrow

60f37a2

[core.sync.mutex] Change assert(0, msg) to abort(msg); improve error …

0291197

…messages

[core.sync.mutex] Cleanup existing unittest

5e4b740

[core.sync.mutex] Add a @safe ddoc-ed test and a @nogc nothrow one

ff38e5e

[core.sync.mutex] Add a test for non-shared use of Mutex

ac71591

[core.sync.mutex] Abort if pthread_mutexattr_destroy fails

90b1db6

* Also make the error messages more descriptive

PetarKirov force-pushed the add-attributes-to-core-sync-mutex branch from 2ceb5cc to 90b1db6 Compare January 24, 2017 20:17

jmdavis approved these changes Jan 25, 2017

View reviewed changes

andralex suggested changes Jan 25, 2017

View reviewed changes

[core.sync.mutex] Address Andrei's comments

7e5d669

andralex merged commit 763343d into dlang:master Jan 26, 2017

PetarKirov deleted the add-attributes-to-core-sync-mutex branch January 26, 2017 15:43

MartinNowak reviewed Jan 30, 2017

View reviewed changes

This was referenced Jul 26, 2017

core.mutex: @nogc lock/unlock #1889

Closed

let initOnce take a shared Mutex dlang/phobos#5661

Merged

PetarKirov mentioned this pull request Jan 25, 2021

Fix issue 21578 - core.atomic.atomicFetchSub for pointers incorrectly… #3343

Merged

Uh oh!

Comments

Conversation

PetarKirov commented Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov commented Jan 14, 2017

Uh oh!

PetarKirov commented Jan 14, 2017

Uh oh!

andralex commented Jan 14, 2017

Uh oh!

andralex commented Jan 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Jan 14, 2017

Uh oh!

PetarKirov commented Jan 14, 2017

Uh oh!

PetarKirov commented Jan 18, 2017

Uh oh!

jmdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov commented Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

PetarKirov commented Dec 28, 2016 •

edited

Loading

PetarKirov Jan 13, 2017 •

edited

Loading

PetarKirov Jan 14, 2017 •

edited

Loading

PetarKirov Jan 14, 2017 •

edited

Loading

PetarKirov commented Jan 25, 2017 •

edited

Loading

PetarKirov Jan 25, 2017 •

edited

Loading

andralex commented Jan 25, 2017 •

edited

Loading