extend logging on DeleteNoThrow #9540
Conversation
There was a problem hiding this comment.
The only other thing I can think of is some sort of "shell out to lsof before/after to collect information about whether another process/user has the file open".
strace to get the low-level syscalls and their results would also be nice but I don't know of a good way to get that from within msbuild, I think it'd need to be in the calling infrastructure.
| string error = process.StandardError.ReadToEnd(); | ||
|
|
||
| process.WaitForExit(); | ||
| if (!string.IsNullOrEmpty(output)) |
There was a problem hiding this comment.
I'm not sure that there's a better answer for this part, but do not that this may rearrange the output and the error.
| Log.LogErrorFromException(ex, showStackTrace: true, showDetail: true, destinationFileState.Name); | ||
| if (NativeMethodsShared.IsLinux) | ||
| { | ||
| Log.LogMessage($"Run lsof before DeleteNoThrow with IsIoRelatedException condition: {destinationFileState.Name}"); |
There was a problem hiding this comment.
I'm not clear on the benefit of including this in the catches. Either it successfully ran in the try part, in which case this is just duplicate information, or it failed in the try, and there's no reason to suspect it will succeed here?
| RunLsof(); | ||
| } | ||
|
|
||
| Log.LogErrorFromException(ex, showStackTrace: true, showDetail: true, destinationFileState.Name); |
There was a problem hiding this comment.
Any reason this is better than 'throw;' ?
| } | ||
| catch | ||
| { | ||
| Log.LogWarning("lsof invocation has failed."); |
There was a problem hiding this comment.
I'm not convinced at the moment that failing in this kind of logging really deserves to take a no-error, no-warning case and turn it into an error/warning case...
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.LogErrorFromException(ex, showStackTrace: true); |
|
Also, have you tried to measure the performance of this? I suspect that building ~anything that includes overwriting a bunch of files will suddenly be very slow with this change as-is. |
This is a draft PR just for collecting more logs for the existing problem on App Services specifically. The sdk with the patched msbuild was already given to the team which experiences this error. Of course, we don't plan to merge these changes, but that you for your comments in any case :) |
Context
It's an attempt to extend logging to understand the nature of this bug
#9250