Convert non-seekable PackagePart streams to seekable #1311
Conversation
ghost
requested review from
…ying previous seekable stream fix to use new functions.
…eams and to ensure we close non-seekable streams once we've fully read them into memory.
… that will create a seekable stream when appropriate.
rladuca
force-pushed
the
dev/roladuca/wrappackagestreams
branch
from
aa39f73 to
4ed3f7b
Compare
src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/IO/Packaging/PackagePartExtensions.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/IO/Packaging/PackagePartExtensions.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
After seeing #1363 I realize this PR may not have considered the consequences of wrapping writable streams. If this is just wrapping in a MemoryStream then there needs to be someone who transfers the data to the original stream, otherwise the writes will never happen.
| @@ -131,7 +131,7 @@ internal void SetCertificate(X509Certificate2 certificate) | |||
| Byte[] byteArray = _certificate.GetRawCertData(); | |||
|
|
|||
| // FileMode.Create will ensure that the stream will shrink if overwritten | |||
| using (Stream s = _part.GetStream(FileMode.Create, FileAccess.Write)) | |||
| using (Stream s = _part.GetSeekableStream(FileMode.Create, FileAccess.Write)) | |||
There was a problem hiding this comment.
who writes back the writable stream?
| @@ -937,7 +937,7 @@ private Stream GetRelationshipStream(PartManifestEntry partEntry) | |||
| private void UpdatePartFromSignature(Signature sig) | |||
| { | |||
| // write to stream | |||
| using (Stream s = SignaturePart.GetStream(FileMode.Create, FileAccess.Write)) | |||
| using (Stream s = SignaturePart.GetSeekableStream(FileMode.Create, FileAccess.Write)) | |||
There was a problem hiding this comment.
who writes back the writable stream?
| /// <param name="mode">The FileMode to open the PackagePart</param> | ||
| /// <param name="access">The FileAccess used to open the PackagePart</param> | ||
| /// <returns>A seekable stream representing the data in the PackagePart.</returns> | ||
| internal static Stream GetSeekableStream(this PackagePart packPart, FileMode mode, FileAccess access) |
There was a problem hiding this comment.
Does it even make sense to specify FileMode/FileAccess, shouldn't it always be open/read? When wrapping a writable stream into a MemoryStream who will be forwarding the writes to the original stream?
Look at the implementation of the extension method. A writeable stream is already seekable when it comes back from PackagePart.GetStream. We just forward those streams onto the caller and do not read them out. The only case where PackagePart.GetStream returns a non-seekable stream is in read-only scenarios. That is precisely why the workaround is to open your XPS or Package as write or read-write. |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using MS.Internal.WindowsBase; |
There was a problem hiding this comment.
Can you move the other PackagePart extension method in WindowsBase\MS\Internal\IO\Packaging\PackagingExtensions.cs to this file?
public static ContentType ValidatedContentType(this PackagePart packagePart)
{
return new ContentType(packagePart.ContentType);
}
Create extension methods for PackagePart that allow us to acquire a seekable stream in the case where the stream returned from PackagePart.GetStream is not already seekable. Switch instances where we call PackagePart.GetStream to use this in order to work-around the fact that a read-only PackagePart stream will use DeflateStream, which is not seekable in .NET Core.
ghost
locked as resolved and limited conversation to collaborators
Fixes #585