Sometimes sending to a Channel will segfault

[johnnovak]

This is happening quite regularly on Windows and the results are catastrophic... I'm actually using a shared channel for this, so I guess it's either a bug in the channel implementation or maybe a GC bug? I'm sending to my shared channel from multiple threads, my understanding is that that's OK as channels use locking internally. The same code has been working flawlessly on Linux and OS X so far.

I am happy to share my full source with the brave person who's willing to investigate this. Isolating the root cause would be extremely hard as I have really no idea where the problem is coming from, and changing anything might actually make it go away.

channels.nim(202)        send
channels.nim(166)        rawSend
alloc.nim(650)           alloc0
alloc.nim(644)           alloc
alloc.nim(481)           rawAlloc
alloc.nim(437)           getSmallChunk
alloc.nim(428)           getBigChunk
alloc.nim(399)           splitChunk
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

[johnnovak]

Another one... As the exact place of the segfault seems to be random, I have a theory that the GC attempts to free the pointer to my shared channel that I passed in in the thread args. Although I have no idea why, because it was allocated using sharedAlloc0....

workerpool.nim(366)      shutdown
workerpool.nim(248)      sendCmd
channels.nim(202)        send
channels.nim(166)        rawSend
alloc.nim(650)           alloc0
alloc.nim(644)           alloc
alloc.nim(481)           rawAlloc
alloc.nim(437)           getSmallChunk
alloc.nim(428)           getBigChunk
alloc.nim(407)           splitChunk
alloc.nim(337)           listAdd
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

[johnnovak]

Btw, compiling with -d:useSysAssert and -d:useGcAssert will make the code fail immediately. Although I'm not too sure if these errors are valid or not because I'm getting incRef: interiorPtr even if I deleted the whole body of my thread proc and just echoed a string...

[GCASSERT] incRef: interiorPtr
[GCASSERT] incRef: interiorPtr
[GCASSERT] incRef: interiorPtr
Traceback (most recent call last)
workerpool.nim(71)       threadProc
assign.nim(96)           genericAssign
assign.nim(85)           genericAssignAux
assign.nim(24)           genericAssignAux
assign.nim(21)           genericAssignAux
assign.nim(85)           genericAssignAux
assign.nim(24)           genericAssignAux
assign.nim(21)           genericAssignAux
assign.nim(91)           genericAssignAux
gc.nim(285)              unsureAsgnRef
gc.nim(117)              incRef
[GCASSERT] incRef: interiorPtr
[GCASSERT] incRef: interiorPtr
Traceback (most recent call last)
workerpool.nim(71)       threadProc
assign.nim(96)           genericAssign
assign.nim(85)           genericAssignAux
assign.nim(24)           genericAssignAux
assign.nim(21)           genericAssignAux
assign.nim(85)           genericAssignAux
assign.nim(24)           genericAssignAux
assign.nim(21)           genericAssignAux
assign.nim(91)           genericAssignAux
gc.nim(285)              unsureAsgnRef
gc.nim(117)              incRef

[endragor]

I've had similar issues on OS X before I realized that you mustn't share channels as objects between threads. Make sure the channel is either in a global variable or passed as a ptr between threads (use allocShared0 to allocate the memory for it). Also, make sure Thread object is never copied - it causes similar errors, and compiler doesn't check that, even though it could.

You can find an example of how to properly pass Channel as an argument in #4689.

[johnnovak]

Thanks man, I appreciate your help. I was aware of the issues you described and checked out your code example. I was already using channels allocated on the shared heap and I was passing only pointers to the thread proc.

In my desperation I tried to create the threads like in your example (currently I'm just creating normal GC'd objects and I'm passing the pointer, that should work and it does). To my surprise, then the whole thing consistently starting crashing right at startup on all platforms, but to my understanding it should not make any difference at all

Anyway, I'm totally confused now, if you could take a look at my code I'd appreciate it, maybe you can spot something that's wrong.

https://github.com/johnnovak/nim-raytracer/blob/master/src/workerpool.nim

To run the tests that sometimes crash on Windows:

cd test
nim c -r --threads:on workerpooltest.nim

[endragor]

It seems you do copy Thread object and that's what causes these problems. Try either changing Worker to be ptr Thread or change WorkerPool to be ref object.

[johnnovak]

Thanks for the suggestion, I have changed WorkerPool and WorkerArgs to be ref objects and Worker to be ptr Thread, but I'm still having the exact same issue (always works on Linux, randomly crashes on Windows).

I suspect there's something wrong with the Channel implementation that breaks it on Windows under some circumstances. Or maybe I'm using too meany features together in an unsupported way that breaks the runtime... who knows... :/

[Araq]

well without any code snippet I cannot fix anything.

[johnnovak]

Hi Araq, you can have a look at the whole source here. Since you would have to debug it anyway, I don't think there's much point in trying to slim it down. I also have a suspicion that maybe I'm using too many features that aren't really compatible with each other...

https://github.com/johnnovak/nim-raytracer/

You can also run the tests that will fail randomly on Windows:

cd test
nim c -r --threads:on workerpooltest.nim

Actually, I would really appreciate it if you could tell me whether I'm doing something stupid in there that is guaranteed to blow up... I'm especially suspicious about those two closures I used for the callback procs (just search for Closure).

UPDATE: I just ran the tests on OS X at work and sometimes it segfaults inside the closures:

Traceback (most recent call last)
workerpool.nim(89)       threadProc
workerpool.nim(78)       ack
workerpool.nim(203)      ackClosure
workerpool.nim(131)      ackCb
workerpool.nim(121)      sendEvent
unittest.nim(295)        eventCb
unittest.nim(184)        checkpoint
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

Still a bit strange that it crashed with a different error on different operating systems. Anyway, any advice is appreciated, I've been struggling with this for days now... Thanks!

EDIT: fixed repo URL

[Araq]

Oh yeah, right now the T that you can pass to a thread needs to be free of GC'ed memory (including your closures). I'm working on a fix, but perhaps this information enables you to come up with a workaround.

[johnnovak]

Ah, that explains the random crashes then. Thanks for the info, I'm looking forward to the fix. Hopefully that will fix all my problems.

[johnnovak]

@Araq Just wondering, any updates on this?

[Araq]

I wanted to retest on Windows, sorry. Can you retest? Lots of bugs wrt channels have been fixed for 0.15.

[johnnovak]

I could only retest on OS X at the moment because I'm at work, but the issue still persists with the latest dev version. Just keep rerunning the tests as described in #4748 (comment)

Some test runs will succeed, but sooner of later you'll get random errors as described in the above comment (basically, it still fails inside the closure).

But as you hinted earlier, this might not even be a channel bug, more like a GC/thread bug:

Oh yeah, right now the T that you can pass to a thread needs to be free of GC'ed memory (including your closures).

So this definitely needs more work...

EDIT: I've removed "on Windows" from the title because it's actually happening on all platforms (just a bit more likely on Windows).