KirillOsenkov / MSBuildStructuredLog

A logger for MSBuild that records a structured representation of executed targets, tasks, property and item values.
MIT License
1.41k stars 188 forks source link

ArgumentNullException reading records from a binary log #752

Closed jaredpar closed 5 months ago

jaredpar commented 5 months ago

Using the 2.2.155 version of the NuPkg I'm starting to see ArgumentNullExceptions when reading newer log files. The stack of the error is:

StructuredLogger.dll!Microsoft.Build.Logging.StructuredLogger.BuildEventArgsReader.FormatResourceStringIgnoreCodeAndKeyword(string resource, string[] arguments) Line 175   C#
    StructuredLogger.dll!Microsoft.Build.Logging.StructuredLogger.BuildEventArgsReader.GetPropertyReassignmentMessage(string propertyName, string newValue, string previousValue, string location) Line 72  C#
    StructuredLogger.dll!Microsoft.Build.Logging.StructuredLogger.BuildEventArgsReader.ReadPropertyReassignmentEventArgs() Line 1096    C#
    StructuredLogger.dll!Microsoft.Build.Logging.StructuredLogger.BuildEventArgsReader.Read() Line 194  C#
    StructuredLogger.dll!Microsoft.Build.Logging.StructuredLogger.BinLogReader.ReadRecordsFromDecompressedStream(System.IO.Stream decompressedStream) Line 327  C#
>   Basic.CompilerLog.Util.dll!Basic.CompilerLog.Util.BinaryLogUtil.ReadAllCompilerCalls(System.IO.Stream stream, System.Collections.Generic.List<string> diagnosticList, System.Func<Basic.CompilerLog.Util.CompilerCall, bool> predicate) Line 135    C#
    Basic.CompilerLog.Util.dll!Basic.CompilerLog.Util.CompilerLogUtil.TryConvertBinaryLog(System.IO.Stream binaryLogStream, System.IO.Stream compilerLogStream, System.Func<Basic.CompilerLog.Util.CompilerCall, bool> predicate) Line 91   C#
    Basic.CompilerLog.Util.dll!Basic.CompilerLog.Util.CompilerLogUtil.TryConvertBinaryLog(System.IO.Stream binaryLogStream, System.IO.Stream compilerLogStream, System.Collections.Generic.List<string> diagnostics, System.Func<Basic.CompilerLog.Util.CompilerCall, bool> predicate) Line 74  C#
    Basic.CompilerLog.Util.dll!Basic.CompilerLog.Util.CompilerLogUtil.ConvertBinaryLog(System.IO.Stream binaryLogStream, System.IO.Stream compilerLogStream, System.Func<Basic.CompilerLog.Util.CompilerCall, bool> predicate) Line 63  C#
    Basic.CompilerLog.Util.dll!Basic.CompilerLog.Util.CompilerLogUtil.GetOrCreateCompilerLogStream(string filePath) Line 41 C#
    Basic.CompilerLog.dll!Program.<Main>$.__GetOrCreateCompilerLogStream|0_10(System.Collections.Generic.List<string> extra) Line 563   C#
    Basic.CompilerLog.dll!Program.<Main>$.__RunPrint|0_2(System.Collections.Generic.IEnumerable<string> args) Line 164  C#
    Basic.CompilerLog.dll!Program.<Main>$(string[] args) Line 26    C#

Digging into the crash it looks like Strings.PropertyReAssignment is null here. Hard to see all the values in the debugger but seems like every static member in Strings is null here. Looking through the code seems there is an expectation that Strings.Initalize is called before this code path is hit.

Think I can likely fix this on my end by just calling this but never had to do this before. Have I just been getting lucky all this time?

Note: this is not easy to reproduce when running from source. It repros 100% of the time when I use my published global tool. Not sure if running from an apphost is changing anything here. Wanted to add that here in case it helps explain what is going on.

jaredpar commented 5 months ago

@rainersigwald this is almost certainly why complog was failing to run on the customer machine. Their scenario is for a new version of VS, same I used locally, and it's seemingly triggering a new path in the forward compat code here.

KirillOsenkov commented 5 months ago

can you try using 2.2.169? I made a fix recently to always call Strings.Initialize(). Hopefully it will fix your case? Need to update to latest for other reasons anyway.

KirillOsenkov commented 5 months ago

This is the fix I made: https://github.com/KirillOsenkov/MSBuildStructuredLog/commit/83a97d05a5e02af24ad0e418d83b27840cdd8c14

does your version have it?

jaredpar commented 5 months ago

No don't have that in the repro case. Case where it doesn't repro does have it.

jaredpar commented 5 months ago

Just assuming that is the fix then. Thanks!

rainersigwald commented 5 months ago

fyi @JanKrivanek. Thanks @KirillOsenkov!

JanKrivanek commented 5 months ago

fyi @JanKrivanek. Thanks @KirillOsenkov!

Thx for heads-up. Yeah this is what we've been batling with in arcade binlogs redaction step end of last year: https://github.com/KirillOsenkov/MSBuildStructuredLog/issues/736