sillsdev / LfMerge

Send/Receive for languageforge.org
MIT License
2 stars 4 forks source link

System.ArgumentNullException: Value cannot be null #323

Closed megahirt closed 1 year ago

megahirt commented 1 year ago

A follow-up exception when trying to fix stuck projects as documented in #317

Putting project 'orm-ec-flex' on hold due to unhandled exception: 
System.ArgumentNullException: Value cannot be null. (Parameter 'path1')
   at System.IO.Path.Combine(String path1, String path2)
   at SIL.LCModel.Infrastructure.Impl.SharedXMLBackendProvider.CreateOrOpen(String name, Int64 capacity, Boolean createdNew)
   at SIL.LCModel.Infrastructure.Impl.SharedXMLBackendProvider.CreateSharedMemory(Boolean createdNew)
   at SIL.LCModel.Infrastructure.Impl.SharedXMLBackendProvider.StartupInternal(Int32 currentModelVersion)
   at SIL.LCModel.Infrastructure.Impl.BackendProvider.StartupInternalWithDataMigrationIfNeeded(IThreadedProgress progressDlg)
   at SIL.LCModel.Infrastructure.Impl.BackendProvider.StartupExtantLanguageProject(IProjectIdentifier projectId, Boolean fBootstrapSystem, IThreadedProgress progressDlg)
   at SIL.LCModel.LcmCache.<>c__DisplayClass12_0.<CreateCacheFromExistingData>b__0(IDataSetup dataSetup)
   at SIL.LCModel.LcmCache.CreateCacheInternal(IProjectIdentifier projectId, String userWsIcuLocale, ILcmUI ui, ILcmDirectories dirs, LcmSettings settings, Action`1 doThe, Action`1 initialize)
   at SIL.LCModel.LcmCache.CreateCacheFromExistingData(IProjectIdentifier projectId, String userWsIcuLocale, ILcmUI ui, ILcmDirectories dirs, LcmSettings settings, IThreadedProgress progressDlg)
   at LfMerge.Core.FieldWorks.FwProject.TryGetLcmCache() in /home/builder/repo/src/LfMerge.Core/FieldWorks/FwProject.cs:line 97
   at LfMerge.Core.FieldWorks.FwProject..ctor(LfMergeSettings settings, String database) in /home/builder/repo/src/LfMerge.Core/FieldWorks/FwProject.cs:line 31
   at LfMerge.Core.LanguageForgeProject.get_FieldWorksProject() in /home/builder/repo/src/LfMerge.Core/LanguageForgeProject.cs:line 93
   at LfMerge.Core.Actions.TransferMongoToLcmAction.DoRun(ILfProject project) in /home/builder/repo/src/LfMerge.Core/Actions/TransferMongoToLcmAction.cs:line 53
   at LfMerge.Core.Actions.Action.Run(ILfProject project) in /home/builder/repo/src/LfMerge.Core/Actions/Action.cs:line 110
   at LfMerge.Core.Actions.SynchronizeAction.DoRun(ILfProject project) in /home/builder/repo/src/LfMerge.Core/Actions/SynchronizeAction.cs:line 64
   at LfMerge.Core.Actions.Action.Run(ILfProject project) in /home/builder/repo/src/LfMerge.Core/Actions/Action.cs:line 110
   at LfMerge.Program.RunAction(String projectCode, ActionNames currentAction) in /home/builder/repo/src/LfMerge/Program.cs:line 124
hahn-kev commented 1 year ago

Hum, I feel like we need a way to reproduce this in order to move forward with it, otherwise it'll take forever to fix and test and we might break other stuff.

megahirt commented 1 year ago

yes, I agree. Unfortunately I need to resist the temptation to work on this until after LFCon23 :)

rmunn commented 1 year ago

Looked into the code involved. The null is the commit log directory, which is a private class member set in SharedXMLBackendProvider's constructor. It's only set if MiscUtils.IsMono is true (to either /dev/shm to use a memory-mapped file, or to the system temp path if /dev/shm is not present). But the Path.Combine call is also only called if MiscUtils.IsMono is true, so it's a mystery to me how it can be getting called with a null first argument. Nevertheless, that's the code path indicated by that stack trace.

hahn-kev commented 1 year ago

That darn IsMono check again. It sounds like someone needs to go through all the code and update all those checks. In the past IsMono also meant IsLinux or Mac, but now it doesn't and IsMono can be false when it's running on Linux and that's causing issues like this where the IsMono check should actually be an IsLinux check. This is what this PR fixed but only for that one line.

rmunn commented 1 year ago

It's worth noting that I've run into cases where System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform actually gave the wrong result, reporting Windows when LfMerge was running on Linux under Mono. (I don't think this will happen any more under .Net Core). Therefore, it may be worth being slightly paranoid about such checks, and using the approach I found in https://stackoverflow.com/a/38795621 to glean that information from the runtime environement, as follows:

If it's necessary to distinguish between Linux and Android, then some further check might be necessary.

rmunn commented 1 year ago

Oh wait, I just saw that I was looking at some older code in liblcm. There is no longer any mystery about why the code path was followed: in https://github.com/sillsdev/liblcm/pull/272, @hahn-dev changed one of the two checks to MiscUtils.IsLinux but missed that the other one needed to be changed as well. I'll submit another PR to liblcm to fix that.

rmunn commented 1 year ago

https://github.com/sillsdev/liblcm/pull/280 will fix this issue. Haven't tested it yet, but I'm 100% confident that this will work.

hahn-kev commented 1 year ago

ooh I see. Nice find. Though I do hope this doesn't just bubble up to another issue.

hahn-kev commented 1 year ago

resolved by #331