Backend(,Backend.Tests): improve marshalling to fix crash #247
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the 1st part (out of three) to fix bug 242: in certain circumstances, the call to Type.GetType(fullTypeName) was returning null (and then causing an NRE like the stacktrace in bug 242 proves) or a FileLoadException [3]. It turns out that .NET's BCL, despite having separate properties for the Type names "FullName" vs "AssemblyQualifiedName", the first one still adds assembly qualified names to types that are part of a generic type definition. I.e., it will not add them in a type such as "System.String" but it will in one such as "List<String>" (see [1]): ``` "System.Collections.Generic.List`1[[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]] ``` This could be dangerous because it would embed the version number (even though we already have a JSON field for that in our MarshallingWrapper type, which could make the type be not found by Type.GetType (and as a consequence return null, or throw the exception mentioned before). The fix, or rather workaround, here, is to use ToString() instead of the .FullName property, as hinted by this S.O. answer: [2]. [1] https://learn.microsoft.com/en-us/dotnet/api/system.type.fullname [2] https://stackoverflow.com/a/4662878/23158491 [3] Stack trace: ``` System.IO.FileLoadException: Could not load file or assembly 'GWallet.Backend, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'. The located assembly's manifest definition does not match the assembly reference. (0x80131040) File name: 'GWallet.Backend, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, StackCrawlMarkHandle stackMark, ObjectHandleOnStack assemblyLoadContext, ObjectHandleOnStack type, ObjectHandleOnStack keepalive) at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, StackCrawlMark& stackMark, AssemblyLoadContext assemblyLoadContext) at System.RuntimeType.GetType(String typeName, Boolean throwOnError, Boolean ignoreCase, StackCrawlMark& stackMark) at System.Type.GetType(String typeName) at GWallet.Backend.Marshalling.ExtractType(String json) in /Users/knocte/Documents/Code/geewalletSTABLE/src/GWallet.Backend/Marshalling.fs:line 156 at GWallet.Backend.Account.ImportSignedTransactionFromJson(String jsonOrCompressedJson) in /Users/knocte/Documents/Code/geewalletSTABLE/src/GWallet.Backend/Account.fs:line 713 at GWallet.Backend.Account.LoadSignedTransactionFromFile(String filePath) in /Users/knocte/Documents/Code/geewalletSTABLE/src/GWallet.Backend/Account.fs:line 766 at Program.BroadcastPayment() in /Users/knocte/Documents/Code/geewalletSTABLE/src/GWallet.Frontend.Console/Program.fs:line 62 at Program.PerformOperation(UInt32 numActiveAccounts, UInt32 numHotAccounts) in /Users/knocte/Documents/Code/geewalletSTABLE/src/GWallet.Frontend.Console/Program.fs:line 398 at Program.ProgramMainLoop[a]() in /Users/knocte/Documents/Code/geewalletSTABLE/src/GWallet.Frontend.Console/Program.fs:line 478 at Program.NormalStartWithNoParameters() in /Users/knocte/Documents/Code/geewalletSTABLE/src/GWallet.Frontend.Console/Program.fs:line 490 ```
Somehow, even if CommonAssemblyInfo.fs is included in Backend.NetStandard project, it was not being applied (as the recent snap testing commit [1] revealed, when it calls geewallet with the --version flag), so we now use MSBuild attributes instead, which work and the best proof that they do, is that we've had to change a LOC in Backend.Tests for them not to break after this change. NOTE: this commit was decided to be backported from master to stable branch, cherry-picking [2], where Backend.NetStandard project doesn't exist, but Backend project does. This is the 2nd part of the bugfix for bug 242. [1] ce93f84 [2] 2edb96e
This is the 3rd and last part of the bugfix for bug 242, which makes sure that Type.GetType() won't return null again, or if it does, that we will have enough info (or innerEx with info) to find culprits and make better decisions later.
|
CI is red because of nblockchain/conventions#146 so I'm gonna ignore it and merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #242