Open-XML-SDK

Open-XML-SDK copied to clipboard

When executing SpreadsheetDocument.Save() (i.e. base class OpenXmlPackage.Save()) I'm getting the following error: InvalidOperationException Collection was modified; enumeration operation may not execute. StackTrace: [DBG]: PS ...>> $_.Exception.InnerException.StackTrace at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource) at System.Collections.Generic.Dictionary`2.Enumerator.MoveNext() at DocumentFormat.OpenXml.Features.PackageFeatureBase.UpdateCachedItems() at DocumentFormat.OpenXml.Packaging.Builder.SavePackageExtensions.SaveablePackage.Save() at DocumentFormat.OpenXml.Packaging.OpenXmlPackage.Save()

This happens only when I'm using the library in a PowerShell script. While debugging in JetBrains Rider (on the same machine) it works OK. It looks like something about the 'Cached Items' is handled differently when library is used after installing in 'C:\Program Files\PackageManagement\NuGet\Packages', comparing to debugging it in IDE. Has anyone encountered this issue?

Environment: Windows Server 2016 PowerShell 5.1 IDE: JetBrains Rider 2023.3.3 DocumentFormat.OpenXml.3.0.0

I stepped through PackageFeatureBase.UpdateCachedItems() in DocumentFormat.OpenXml.Features internal abstract class PackageFeatureBase ... protected void UpdateCachedItems() { if (this._parts == null) return; foreach (KeyValuePair<Uri, PackageFeatureBase.PackagePart> part in this._parts) { if (this.Package.PartExists(part.Key)) part.Value.Part = this.Package.GetPart(part.Key); else this._parts.Remove(part.Key); } } I'm not an expert, but I thought you're not supposed to remove keys from a Dictionary while looping over it.

Anyway, my code does reach the 'Remove' part and deletes removed parts. I'm not sure if this should be happening. I might be doing something wrong earlier, maybe those obsolete parts need to be somehow handled explicitly, not sure how though.

@twsouthwick can you take a look?

@SirParser can you provide a repro for me?

Thanks @AlfredHellstern and @twsouthwick attached steps to reproduce the error Repro.zip

Oh - you're loading it manually through powershell and on an older version of powershell. You should be resolving the .net framework assemblies if you want to do as you're on Powershell 5.1

However, resolving them manually is difficult given powershell vs powershell core. We could potentially package them up as a powershell module so that things are resolved correctly. I haven't done that for a while, but I'd be open for taking a PR to enable an official powershell module that would set things up correctly.

For more technical details, on .NET 6, enumerating over a dictionary that has had items removed is now supported. So, we special case our compilation to have a different pathway for .NET 6+ vs others: https://github.com/dotnet/Open-XML-SDK/blob/35b91d3c18fc06379b74f0980e3498f2b04c1fc8/src/DocumentFormat.OpenXml.Framework/Features/PackageFeatureBase.cs#L46-L82

Thanks @twsouthwick.

Please excuse my ignorance, but my understanding so far was that netstandard2.0 dll's should work fine regardless of the target .Net environment. I'm loading the dll's manually in PowerShell because otherwise it doesn't know where to take them from.

Can you direct me to some link, or share more info that explains this in more detail:

You should be resolving the .net framework assemblies if you want to do as you're on Powershell 5.1

From what you wrote I understand that my PS script uses wrong version of PackageFeatureBase. It uses version with code designed for NET6_0_OR_GREATER (with direct key removal instead of via List<Uri>? unused). I don't understand why this is so considering I'm explicitly loading the netstandard2.0 dll version.

How can I force the PS script to use the correct version of dll? Is it something along the lines of: https://stackoverflow.com/questions/23268475/resolving-assembly-dependency-references-with-powershell

Or should I add .Net Framework 4.6 to the target frameworks of my library and use it instead of netstandard2.0? But then I thought netstandard2.0 is smart enough to work with whatever version of .Net ...

I'll be out of town the next few days but let me try to answer these.

. NET standard in general works on all platforms but we have a specific framework build for actually running it on framework. This is due to a number of weirdness with the Windowsbase.dll implementation of the packaging Apis. So, things probably won't work well to use the net standard build on framework. I've never tested it and we expect people to use nuget to resolve the best platform assemblies.

I would try resolving the framework version of the assemblies and see if that works.

I'm not opposed to an official powers he'll module to simplify this resolution so if you want to submit a PR, happy to consider it.

The specific issue you're hitting is a weird one to hit even with the standard libs - I wouldn't expect it. I haven't had time to repro, but will try end of next week when I'm back

Hi Taylor,

I haven't had time to look into this in the last few months, but some requirement popped up recently and I ran into this issue again. This time I wanted to use an Excel template with a table and that triggered again the UpdateCachedItems() method and the "Collection was modified..." error. I started pulling my hair out again trying to get the right versions and frameworks in place but to little avail. At least I kind of confirmed that the intended pre-NET.6 version of that method is being executed. Then I looked at the pre-NET.6 compiled code (and your code snippet above) and I realized that it doesn't actually address the dictionary item removal issue, because despite using an additional temp List ('unused') of keys (Uris), it still removes the OpenXmlPart within the original loop. For this to work, the removal part using the 'unused' List has to happen outside of the original loop over '_parts'. Can you check my quick mockup in attachment and let me know if that makes sense? Test.txt (it has to run in a .NET Framework 4.5+ Console App)

Regards, PM

Here's the proposed change:

	protected void UpdateCachedItems() 
	{ 
		if (_parts is null) 
		{ 
			return; 
		} 
  
 #if !NET6_0_OR_GREATER 
        List<Uri>? unused = null; 
 #endif 
  
		foreach (var part in _parts) 
		{ 
			if (Package.PartExists(part.Key)) 
			{ 
				part.Value.Part = Package.GetPart(part.Key); 
			} 
			else 
			{ 
 #if NET6_0_OR_GREATER 
				_parts.Remove(part.Key); 
 #else 
				(unused ??= new()).Add(part.Key); 
 #endif 
			} 
// CHANGE START
		} 
 #if !NET6_0_OR_GREATER 
		if (unused is not null) 
		{ 
			foreach (var uri in unused) 
			{ 
				_parts.Remove(uri); 
			} 
		} 
 #endif 
// CHANGE END
	} 

@twsouthwick can you have a look?