Race conditions in cleanup code

(I mentioned this on irc, but you also get a proper report.)

Problem

createRedirectedProcess contains this bit:

Lines 103 to 107 in 29b83cf

    
           void $ liftIO $ forkIO $ waitForProcess ph >>= \ec -> mask_ $ do 
        
             ecTrigger ec 
        
             P.cleanupProcess po 
        
             killThread outThread 
        
             killThread errThread

I think there are problems with this:

There is no guarantee that the output threads forked in lines 92 and 98 above have finished processing. This is most relevant if the started process prints interesting info right before quitting on its own accord.

reflex-process/src/Reflex/Process.hs

Lines 89 to 101 in 29b83cf

    
           let output h = do 
        
                 (e, trigger) <- newTriggerEvent 
        
                 reader <- liftIO $ mkReadStdOutput h trigger 
        
                 t <- liftIO $ forkIO reader 
        
                 return (e, t) 
        
           let err_output h = do 
        
                 (e, trigger) <- newTriggerEvent 
        
                 reader <- liftIO $ mkReadStdError h trigger 
        
                 t <- liftIO $ forkIO reader 
        
                 return (e, t) 
        
           (out, outThread) <- output hOut 
        
           (err, errThread) <- err_output hErr

As a consequence, cleanupProcess closing handles means that these threads may run into exceptions ("read on closed handle") but they also get killed. So the network might not get events for stdout/stderr happening right before child process shutdown, plus a potential exception.
Also, why mask the ecTrigger ec? Maybe this is harmless, but I don't see the benefit to masking in this fashion - waitForProcess is not masked anyway, so you get little guarantee. I assume that waitForProcess is interruptible so I could see a purpose in masking the whole block. Not sure though, async exceptions are always tricky.

Relevance

For "daemon" type child processes the existing code probably works fine, but if you start short-lived processes via this interface, it is definitely possible to not get any output captured as a consequence of this. I observed this already. And this error is silent, too.

Proposed Solution

Afaict the only way to ensure that the forked threads have finished doing their job is waiting until they terminate. After that, you should be safe to kill them :) and to close the handles. But really, the threads should have a finally-block that does the closing. For the input handle (stdin of the child process) it is fine to close the handle when the child process has finished.

Good catch. We do want to terminate the reading threads, but we need to ensure that the process is terminated before killing them otherwise the process can block (trying to write). We also need to wait for the process output Event itself to be finalized (no more subscribers to the Event) because the reading threads could continue producing output after the process is terminated. newEventWithLazyTriggerWithOnComplete seems to be the tool we need for this.

The Events for reading output should also signal when they see an EOF like suggested in #7.

Just looked at this again. Can't we just not kill the threads at all and let them die naturally when the handles are closed? That would make this much simpler.

	void $ liftIO $ forkIO $ waitForProcess ph >>= \ec -> mask_ $ do
	ecTrigger ec
	P.cleanupProcess po
	killThread outThread
	killThread errThread

	let output h = do
	(e, trigger) <- newTriggerEvent
	reader <- liftIO $ mkReadStdOutput h trigger
	t <- liftIO $ forkIO reader
	return (e, t)

	let err_output h = do
	(e, trigger) <- newTriggerEvent
	reader <- liftIO $ mkReadStdError h trigger
	t <- liftIO $ forkIO reader
	return (e, t)
	(out, outThread) <- output hOut
	(err, errThread) <- err_output hErr