Is it safe to return a view to the contents of a smartptr?

[planetis-m]

Nope. needs Isolated[T] no idea if that would work.

[shayanhabibi]

Not at all. Isolated would at most specify that at the time of isolation, no other thread has a link to it AFAIK. It has nothing to do with threading afaic.

There's no sync or memory coherence on the contents of the smartptr. I think it would be better to roll atomic counting into ref objects someway.

[planetis-m]

So I am no way an expert on this, but in my mind there are two separate issues here. First one as you put it, is two or more threads sharing the same smart pointer. Indeed there is no synchronization going on and it's left up to the user to do the synchronization, like in the examples. Reason for choosing this was reading this article (Herb Sutter's blog will update when I find it) about choosing the right level of abstraction when doing synchronization.

Second, is about Nim's 'isolate' construct and how it can be easily by-passed (which is what this issue is about). So on top of my head is this example:

# thread 1
let temp = mySharePtr.refField[] # returns `var T`, meaning you can mess with the `ref`'s object count, should create a fresh copy instead

[shayanhabibi]

I'm not sure if you've run into this issue, but arc is not compatible with concurrency. If you allocate ref objects on one thread and consume them rapidly in parallel on multiple other threads, you will eventually hit a SIGSEGV from the allocating thread using dirty memory. Unlikely to occur with real work being done inbetween but certainly of concern since any possibility of undefined behaviour sours the experience.

I've made a discussion on it in nim skull.

If you are able to find a way to fix this without changing arc I will be interested to know

[planetis-m]

As you can see in https://github.com/planetis-m/sync/blob/master/tests/tspsc1.nim I can pass a ref object between threads without an issue. I suspect with orc, this is harder to do, judging from this nim-lang/threading@8bd3e3b#diff-4f106f059e9b3154ca56e6ba0efac1382a1f2c699153786217fa34cbc8b8ccfaR368

[shayanhabibi]

I think you misunderstand; as with all things to do with concurrency/parallelism, the undefined behaviour still exists. If you try with more threads on a MPMC queue you will more likely hit this issue. It is highly unlikely the SPSC linked can sufficiently recreate the issue.

I'm sure you can agree that arc not being atomic instantaneously makes it vulnerable to undefined behaviour.

If you use asan on it you will see this.

[shayanhabibi]

https://github.com/nim-works/loony/tree/main/failtests

You can run these tests to demonstrate the behaviour. Let me know what you think.

[planetis-m]

My friend, I run all the tests with TSan, there is no undefined behavior. You might need to borrow the logic from threading\channels for your send and receive procs (the stuff with isolate).

[shayanhabibi]

==================
WARNING: ThreadSanitizer: data race (pid=2289)
  Write of size 8 at 0x7b30000000a8 by thread T2 (mutexes: write M7):
    #0 sendMpmc__OOZthreadingZchannels_232 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1ba84)
    #1 channelSend__tchannels95simple_194 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1e579)
    #2 send__tchannels95simple_178 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1e83f)
    #3 secondWorker__tchannels95simple_208 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1f3b5)
    #4 threadProcWrapDispatch__system_3573 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x18ae8)
    #5 threadProcWrapStackFrame__system_3563 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x18d9f)
    #6 threadProcWrapper__system_3549 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x18df7)

  Previous read of size 8 at 0x7b30000000a8 by main thread:
    #0 recvMpmc__OOZthreadingZchannels_282 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1c754)
    #1 channelReceive__tchannels95simple_238 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1ed2e)
    #2 tryRecv__tchannels95simple_271 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1f51b)
    #3 NimMainModule <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x20ad3)
    #4 NimMainInner <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x2050c)
    #5 NimMain <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x20575)
    #6 main <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x205fd)

  As if synchronized via sleep:
    #0 nanosleep ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:362 (libtsan.so.0+0x63360)
    #1 nossleep <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1de04)
    #2 secondWorker__tchannels95simple_208 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1f2c2)
    #3 threadProcWrapDispatch__system_3573 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x18ae8)
    #4 threadProcWrapStackFrame__system_3563 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x18d9f)
    #5 threadProcWrapper__system_3549 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x18df7)

  Location is heap block of size 192 at 0x7b3000000000 allocated by main thread:
    #0 malloc ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:651 (libtsan.so.0+0x30323)
    #1 allocChannel__OOZthreadingZchannels_131 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x19de4)
    #2 newChan__tchannels95simple_3 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1e261)
    #3 NimMainModule <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x20712)
    #4 NimMainInner <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x2050c)
    #5 NimMain <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x20575)
    #6 main <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x205fd)

  Mutex M7 (0x7b3000000000) created at:
    #0 pthread_mutex_init ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1220 (libtsan.so.0+0x4a616)
    #1 initLock__coreZlocks_64 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x19840)
    #2 allocChannel__OOZthreadingZchannels_131 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x19ef3)
    #3 newChan__tchannels95simple_3 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1e261)
    #4 NimMainModule <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x20712)
    #5 NimMainInner <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x2050c)
    #6 NimMain <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x20575)
    #7 main <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x205fd)

  Thread T2 (tid=2292, running) created by main thread at:
    #0 pthread_create ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:962 (libtsan.so.0+0x5ea79)
    #1 createThread__system_3528 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1908d)
    #2 createThread__system_3519 <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x19222)
    #3 NimMainModule <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x209ba)
    #4 NimMainInner <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x2050c)
    #5 NimMain <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x20575)
    #6 main <null> (tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x205fd)

SUMMARY: ThreadSanitizer: data race (/home/shayan/.cache/nim/tchannels_simple_d/tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A+0x1ba84) in sendMpmc__OOZthreadingZchannels_232
==================
ThreadSanitizer: reported 1 warnings
Error: execution of an external program failed: '/home/shayan/.cache/nim/tchannels_simple_d/tchannels_simple_BDEB89ECC21CBB3EBB997D5C2ADCC5280B56155A '

That was my output for running the Chan test. What do you get? Yes using isolate might be safer. But theres many issues with isolate.

It's inflexible. There's no way for isolate to be used in any substantial scenario that is worth it in my opinion. Isolate does not make arc concurrent safe. Take arc, take loony, take everything out of it. At its base, arc is not thread safe. It is full of undefined behaviour. My tests which resulted in the undefined behaviour were the result of testing on large static arrays with simple atomic operations to make it the fastest possible.

I also don't accept locks/mutexes as being viable in my mode of operations as they are too slow.

[planetis-m]

Please let's avoid posting about loony issues in this repo from now on. isolate in my experience works, a little clumsy to use, but it can be hidden from the user with templates. The linked spsc test passes the same ref object back and forth between two threads. There is no undefined behavior!

[shayanhabibi]

Please do not assume things like where the issue came from! This issue was discovered on a static array MPMC. I never referenced loony until you gave me inaccurate and insufficient tests to demonstrate that arc is safe.

There is undefined behavior. Arc has undefined behavior. That much is very obvious. How does a non-atomic ref count not have undefined behavior?

[planetis-m]

So the most obvious thing for me is GC_ref and GC_unref, which I assume you put in order to workaround the =destroy calls the compiler inserts at the end of push and =copy at pop (if I remember correctly) (which means their ref count is modified by another thread and thus your nag that arc is not thread safe). With isolate you basically cancel out these unwanted interactions. Sorry if my information is inaccurate, its been a lot of months since I last worked on sync. I will try re running the tests with latest nim, but I doubt anything changed, they are still thread-safe.

[shayanhabibi]

Incorrect. That is not why I am "nagging". You can read what I have said multiple times to understand where the issue is. I don't have any issues with the GC_ref and GC_unref since I use thread fences. You don't understand the issue at hand. Don't worry about it then.

[planetis-m]

Thank you for being kind from now on but let me remind you that =destroy happens after the threadfence. Please refrain from commenting here about my knowledge of multithreading, have a nice day.

[planetis-m]

Let my recap the exact issue, I track here and nothing else, suppose the SharedPtr contains a regular ref object. Provided that you accessed/modified it's content in a thread-safe manner (using a Lock or Monitor, etc) and you accidentally created a copy of the said object INSIDE it's aforementioned guarded by lock access/modification, you have potentially created a data race. As I tried but failed to express in the issue, proc get: var T makes no effort to guarantee a =copy on the ref object cannot be made. I suppose proc get: Isolated[var T] cannot work either.

var b: SharedPtr[ref Foo]

proc thread1 {.thread.} =
  var a: ref Foo
  withLock L:
     a = b[] # boom, there is a `=copy` right here, should have been a .cursor
  # now on proc exit the =destroy call on `a` decrements the ref count of `ref Foo` and creates a data race.

[planetis-m]

needs a big warning in the docs for now. Another idea was to have a construct like proc =copy(dest: ref T src: SharedPrt[ref T]) {.error.} but that obviously doesn't work.