sillsdev / chorus

End-user collaboration library via 3-way xml merging and hg dvcs under the hood
6 stars 26 forks source link

Don't throw NullReferenceException in GetTextFromQuery #344

Closed rmunn closed 2 weeks ago

rmunn commented 1 month ago

Occasionally, though I've been unable to reproduce it consistently*, HgRepository.ExecuteErrorsOk can produce a result whose StandardOutput or StandardError is null instead of an empty string. When that happens, the result.StandardOutput.Trim() or result.StandardError.Trim() call in GetTextFromQuery will throw a NullReferenceException.

* Last time I managed to reproduce it was by re-running the same LfMerge test suite 60 times before it finally happened.

We should ensure that GetTextFromQuery can gracefully handle a null stdout or stderr and treat it as if it were an empty string.

rmunn commented 1 month ago

The command that failed, incidentally, was:

hg heads --template "changeset:{rev}:{node|short}
branch:{branches}
user:{author}
date:{date|rfc822date}
tag:{tags}
summary:{desc}
"

Normally its stderr would be an empty string, but in this case stderr was null instead.

rmunn commented 1 month ago

It looks like the root cause might be a race condition in HgProcessOutputReader.Read(). It launches two separate threads calling HgProcessOutputReader.ReadStream() on the stdout and stderr of the hg process. It then waits for both threads to be in the ThreadState.Stopped state before reading the .Results property of outputReaderArgs and errorReaderArgs and assigning those to its StandardOutput and StandardError properties.

ReadStream(), in turn, uses a try ... finally block, writing to readerArgs.Results in the finally block. So until ReadStream() completes, readerArgs.Results is null.

The assumption based into this code is that ThreadState.Stopped is reliable, and always signals that the thread has completely finished (including any finally blocks). However, my experience is that this is not always the case, and it is possible for the thread scheduler to put a thread into the ThreadState.Stopped state before its finally block has finished. The C# documentation warns (emphasis mine):

The ThreadState enumeration defines a set of all possible execution states for threads. It's of interest only in a few debugging scenarios. Your code should never use the thread state to synchronize the activities of threads.

In this particular case, since readerArgs.Results is null until the thread has finished running its finally block, it's safe to use Results == null as the proxy for ThreadState.Stopped, and I believe this will get better results overall.