search

JohnSundell / ShellOut

Easily run shell commands from a Swift script or command line tool
MIT License
872 stars 85 forks source link

Data races when concurrently running shellOut commands #24

Closed harlanhaskins closed 4 years ago

harlanhaskins commented 6 years ago

I've noticed, while trying to add parallel execution to Lite, if you try to execute shellOut on multiple dispatch queues it causes data races inside Process.launchBash. I'm not quite sure how to work around this, but I can try to take a look at a potential fix.

==================
WARNING: ThreadSanitizer: Swift access race (pid=12148)
  Read of size 8 at 0x7b0c0003dc90 by thread T1:
  * #0 Process.launchBash(with:outputHandle:errorHandle:) ShellOut.swift:400 (ShellOut:x86_64+0x445a)
    #1 shellOut(to:arguments:at:outputHandle:errorHandle:) ShellOut.swift:35 (ShellOut:x86_64+0x21ee)
    #2 TestRunner.run(file:) TestRunner.swift:203 (LiteSupport:x86_64+0x27d1b)
    #3 closure #2 in TestRunner.run() TestRunner.swift:90 (LiteSupport:x86_64+0x1d28a)
    #4 partial apply for closure #2 in TestRunner.run() TestRunner.swift (LiteSupport:x86_64+0x1d5b0)
    #5 thunk for @callee_owned () -> (@owned [TestResult]) TestRunner.swift (LiteSupport:x86_64+0x1d72b)
    #6 partial apply for thunk for @callee_owned () -> (@owned [TestResult]) TestRunner.swift (LiteSupport:x86_64+0x1d810)
    #7 closure #1 in ParallelExecutor.addTask(_:) ParallelExecutor.swift:39 (LiteSupport:x86_64+0x5834)
    #8 partial apply for closure #1 in ParallelExecutor.addTask(_:) ParallelExecutor.swift (LiteSupport:x86_64+0x5940)
    #9 thunk for @callee_owned () -> () ParallelExecutor.swift (LiteSupport:x86_64+0x4c3c)
    #10 __wrap_dispatch_group_async_block_invoke <null>:1056912 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x67327)
    #11 _dispatch_client_callout <null>:1056912 (libdispatch.dylib:x86_64+0x1d1e)

  Previous modifying access at 0x7b0c0003dc90 by thread T17:
  * #0 closure #1 in Process.launchBash(with:outputHandle:errorHandle:) ShellOut.swift:364 (ShellOut:x86_64+0xfc2c)
    #1 partial apply for closure #1 in Process.launchBash(with:outputHandle:errorHandle:) ShellOut.swift (ShellOut:x86_64+0xfe2b)
    #2 thunk for @callee_owned (@owned FileHandle) -> () ShellOut.swift (ShellOut:x86_64+0xfec4)
    #3 __33-[NSConcreteFileHandle _monitor:]_block_invoke <null>:1056912 (Foundation:x86_64+0x144ec7)
    #4 _dispatch_client_callout <null>:1056912 (libdispatch.dylib:x86_64+0x1d1e)

  Issue is caused by frames marked with "*".

  Location is heap block of size 40 at 0x7b0c0003dc80 allocated by thread T1:
    #0 malloc <null>:1056944 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x48b3a)
    #1 swift_slowAlloc <null>:1056944 (libswiftCore.dylib:x86_64+0x344478)
    #2 Process.launchBash(with:outputHandle:errorHandle:) ShellOut.swift (ShellOut:x86_64+0x2c13)
    #3 shellOut(to:arguments:at:outputHandle:errorHandle:) ShellOut.swift:35 (ShellOut:x86_64+0x21ee)
    #4 TestRunner.run(file:) TestRunner.swift:203 (LiteSupport:x86_64+0x27d1b)
    #5 closure #2 in TestRunner.run() TestRunner.swift:90 (LiteSupport:x86_64+0x1d28a)
    #6 partial apply for closure #2 in TestRunner.run() TestRunner.swift (LiteSupport:x86_64+0x1d5b0)
    #7 thunk for @callee_owned () -> (@owned [TestResult]) TestRunner.swift (LiteSupport:x86_64+0x1d72b)
    #8 partial apply for thunk for @callee_owned () -> (@owned [TestResult]) TestRunner.swift (LiteSupport:x86_64+0x1d810)
    #9 closure #1 in ParallelExecutor.addTask(_:) ParallelExecutor.swift:39 (LiteSupport:x86_64+0x5834)
    #10 partial apply for closure #1 in ParallelExecutor.addTask(_:) ParallelExecutor.swift (LiteSupport:x86_64+0x5940)
    #11 thunk for @callee_owned () -> () ParallelExecutor.swift (LiteSupport:x86_64+0x4c3c)
    #12 __wrap_dispatch_group_async_block_invoke <null>:1056944 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x67327)
    #13 _dispatch_client_callout <null>:1056944 (libdispatch.dylib:x86_64+0x1d1e)

  Thread T1 (tid=7782461, running) is a GCD worker thread

  Thread T17 (tid=7782485, running) is a GCD worker thread

SUMMARY: ThreadSanitizer: Swift access race ShellOut.swift:400 in Process.launchBash(with:outputHandle:errorHandle:)
==================
harlanhaskins commented 6 years ago

Okay the issue here is with unguarded accesses to the outputData and errorData of the call to launchBash. They're set on a reader thread in the readabilityHandler of the error/outputPipe arguments, but then read from the caller's queue at the end of the function. This can be mitigated by protecting accesses to the outputData and errorData fields with a DispatchQueue. PR incoming.

JohnSundell commented 4 years ago

Was fixed a long time ago, just forgot to close this 😅