Trace2FileWriter: ignore some exceptions on write

[derrickstolee]

These exceptions were discovered while exploring trace2 settings with a full Git client.

Git can take a directory location as a trace2 target and will create new files for every process. GCM currently throws a
DirectoryNotFoundException given such a parameter.

Git processes somehow can append to the same file across multiple subprocesses. When GCM attempts to append to the file, it gets an UnauthorizedAccessException on Windows due to multiple writers being problematic. This exception could also happen on other platforms if the setting is pointing to a file with restricted permissions.

In both of these cases, we chose to do nothing. The traces are lost, but that's better than crashing the process.

Future directions could include:

1. Sending a warning over stderr if these exceptions occur, to make it clear why trace2 are not showing up.

2. Directories could be noticed as a different kind of trace target and we create a new file for the process before passing it to the Trace2FileWriter.

3. Perhaps there is a way for Git to pass the handle to the trace file so we can append to the file that Git was using. (Alternatively, Git could close the file and then reopen it after running the GCM subprocess.)

Each of these issues are a bit complicated, so this quick fix is chosen as a stop gap to avoid this problem for current users.

[jeffhostetler]

if we get that error, we should just disable telemetry so that we won't keep trying to write to the file.

[jeffhostetler]

but that isn't important right now for testing purposes.

[derrickstolee]

if we get that error, we should just disable telemetry so that we won't keep trying to write to the file.

That's effectively what we're doing. It's just that this is the layer where we actually try appending to the file.

An alternative would be to mark this Trace2FileWriter as inactive or something, so we don't even try to write again when a later trace message arrives.

[jeffhostetler]

Where is the call that passes (the equivalent of) O_APPEND to the kernel? AKA FILE_APPEND_DATA ?

[derrickstolee]

Where is the call that passes (the equivalent of) O_APPEND to the kernel? AKA FILE_APPEND_DATA ?

File.AppendAllText() opens the file with O_APPEND, writes the line, then closes the file. This is a bit different from opening the file in the constructor and then using the stream from then on.

It might be worthwhile to rearrange the class to operate that way.

[jeffhostetler]

yeah, marking it inactive is what i was implying.

[mjcheetham]

Where is the call that passes (the equivalent of) O_APPEND to the kernel? AKA FILE_APPEND_DATA ?

File.AppendAllText() opens the file with O_APPEND, writes the line, then closes the file. This is a bit different from opening the file in the constructor and then using the stream from then on.

It might be worthwhile to rearrange the class to operate that way.

I believe this is how @ldennington originally had this, operating on a handle that we'd opened once, but there was lots of problems with ensure writes were flushed (calling Flush didn't work). Ended up with intermingled output lines from Git, GCM, and other GCM processes.

[derrickstolee]

Where is the call that passes (the equivalent of) O_APPEND to the kernel? AKA FILE_APPEND_DATA ?

File.AppendAllText() opens the file with O_APPEND, writes the line, then closes the file. This is a bit different from opening the file in the constructor and then using the stream from then on.
It might be worthwhile to rearrange the class to operate that way.

I believe this is how @ldennington originally had this, operating on a handle that we'd opened once, but there was lots of problems with ensure writes were flushed (calling Flush didn't work). Ended up with intermingled output lines from Git, GCM, and other GCM processes.

Thanks for that context! I will stop my attempts at swapping the order.

[jeffhostetler]

In both of these cases, we chose to do nothing. The traces are lost, but that's better than crashing the process.

Future directions could include:

1. Sending a warning over stderr if these exceptions occur, to make it clear why trace2 are not showing up.

I have git.exe only emit such warnings if GIT_TRACE2_DST_DEBUG or trace2.destinationdebug are set to keep down the noise.

2. Directories could be noticed as a different kind of trace target and we create a new file for the process before passing it to the Trace2FileWriter.

this is what we should do. detect a directory pathname and create a unique log file in the constructor.

3. Perhaps there is a way for Git to pass the handle to the trace file so we can append to the file that Git was using. (Alternatively, Git could close the file and then reopen it after running the GCM subprocess.)

Sharing a file descriptor might work, but there are upstream concerns here.

Closing and re-opening is problematic. We have no guarantees that git.exe is Sync WRT GCM -- there could be other background threads doing things in git.exe. GCM should create it's own file in this case.

In the normal file case, GCM is appending to a flat file that git.exe is also writing to, so we know that the file sharing bits are ok, right?