Fixes #79187 Add MemoryMappedFile.CreateFromFile(SafeFileHandle) overload #83134

amalabey · 2023-03-08T12:23:06Z

The overload uses the handle the perform validations instead of instantiating a FileStream and calling the FileStream overload, because the FileStream instantiation appears to be heavy on allocations.

dotnet-issue-labeler · 2023-03-08T12:23:11Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-03-08T12:23:21Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #79187

Author:	amalabey
Assignees:	-
Labels:	`area-System.IO`, `new-api-needs-documentation`
Milestone:	-

amalabey · 2023-03-08T12:26:10Z

@dotnet-policy-service agree

adamsitnik

The changes look very good to me, thank you for you contribution @amalabey!

I need two things before I hit the approve button:

minor improvements of the xml doc comments (please see my comment)
the conflict needs to be solved

adamsitnik · 2023-03-15T08:14:00Z

src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

+        /// <summary>
+        /// Creates a memory-mapped file from an existing file using a SafeFileHandle,
+        /// and the specified access mode, name, inheritability, and capacity.
+        /// </summary>
+        /// <param name="fileHandle">The SafeFileHandle to the existing file. Caller is
+        /// responsible for disposing <c>fileHandle</c> when <paramref name="leaveOpen"/>==true (otherwise,
+        /// automatically disposed by the <c>MemoryMappedFile</c>). </param>
+        /// <param name="mapName">A name to assign to the memory-mapped file, or <c>null</c> for a
+        /// <see cref="MemoryMappedFile"/> that you do not intend to share across processes.</param>
+        /// <param name="capacity">The maximum size, in bytes, to allocate to the memory-mapped file.
+        /// Specify 0 to set the capacity to the size of <c>filestream</c>.</param>
+        /// <param name="access">One of the enumeration values that specifies the type of access allowed
+        /// to the memory-mapped file.<para>This parameter can't be set to <c>Write</c>.</para></param>
+        /// <param name="inheritability">One of the enumeration values that specifies whether a handle
+        /// to the memory-mapped file can be inherited by a child process. The default is <c>None</c>.</param>
+        /// <param name="leaveOpen">A value that indicates whether to close the source file handle when
+        /// the <see cref="MemoryMappedFile"/> is disposed.</param>
+        /// <returns>A memory-mapped file that has the specified characteristics.</returns>
+        /// <exception cref="ArgumentException">
+        /// <para><c>mapName</c> is <c>null</c> or an empty string.</para>
+        /// <para>-or-
+        /// <c>capacity</c> and the length of the file are zero.</para>
+        /// <para>-or-
+        /// <c>access</c> is set to Write or Write enumeration value, which is not allowed.</para>
+        /// <para>-or-
+        /// access is set to Read and <c>capacity</c> is larger than the length of <c>fileHandle</c>.</para>
+        /// </exception>
+        /// <exception cref="ArgumentNullException"><c>fileHanlde</c> is <c>null</c>.</exception>
+        /// <exception cref="ArgumentOutOfRangeException">
+        /// <c>capacity</c> is less than zero.
+        /// <para>-or-
+        /// <c>capacity</c> is less than the file size.</para>
+        /// <para>-or-
+        /// <c>access</c> is not a valid <see cref="MemoryMappedFileAccess"/> enumeration value.</para>
+        /// <para>-or-
+        /// <c>inheritability</c> is not a valid <see cref="HandleInheritability"/> enumeration value.</para>
+        /// </exception>


The xml docs need some minor improvements:

instead of ==true we need to use <see langword="true" />

instead of <c>null</c> we need to use <see langword="null" />

when refering to parameters we need to use <paramref name="$name" /> syntax

adamsitnik · 2023-03-15T08:18:54Z

src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

+            if (mapName != null && mapName.Length == 0)
+            {
+                throw new ArgumentException(SR.Argument_MapNameEmptyString);
+            }
+
+            if (capacity < 0)
+            {
+                throw new ArgumentOutOfRangeException(nameof(capacity), SR.ArgumentOutOfRange_PositiveOrDefaultCapacityRequired);
+            }
+
+            long fileSize = RandomAccess.GetLength(fileHandle);
+            if (capacity == 0 && fileSize == 0)
+            {
+                throw new ArgumentException(SR.Argument_EmptyFile);
+            }
+
+            if (access < MemoryMappedFileAccess.ReadWrite ||
+                access > MemoryMappedFileAccess.ReadWriteExecute)
+            {
+                throw new ArgumentOutOfRangeException(nameof(access));
+            }
+
+            if (access == MemoryMappedFileAccess.Write)
+            {
+                throw new ArgumentException(SR.Argument_NewMMFWriteAccessNotAllowed, nameof(access));
+            }
+
+            if (inheritability < HandleInheritability.None || inheritability > HandleInheritability.Inheritable)
+            {
+                throw new ArgumentOutOfRangeException(nameof(inheritability));
+            }


the changes you are introduced are perfectly correct. Would you be interested in moving the argument validation logic to a separate method and reusing it in all 3 the CreateFromFile overloads (accepting path, FileStream and SafeFileHandle)?

Absolutely, that makes sense. I will make that refactoring and update the PR.

amalabey · 2023-03-18T03:37:01Z

@adamsitnik Addressed your feedback. I had to mark a test method with ActiveIssueAttribute, because the related CreateFromFile(FileStream) test was failing due to the same reason.

adamsitnik

The code looks very good, PTAL at my comments. Again thank you for your contribution @amalabey !

adamsitnik · 2023-03-21T08:29:17Z

src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

+        /// <summary>
+        /// Creates a memory-mapped file from an existing file using a SafeFileHandle,
+        /// and the specified access mode, name, inheritability, and capacity.
+        /// </summary>
+        /// <param name="fileHandle">The SafeFileHandle to the existing file. Caller is
+        /// responsible for disposing <paramref name="fileHandle"/> when <paramref name="leaveOpen"/> is <see langword="true" /> (otherwise,
+        /// automatically disposed by the <see cref="MemoryMappedFile"/>). </param>
+        /// <param name="mapName">A name to assign to the memory-mapped file, or <see langword="null" /> for a
+        /// <see cref="MemoryMappedFile"/> that you do not intend to share across processes.</param>
+        /// <param name="capacity">The maximum size, in bytes, to allocate to the memory-mapped file.
+        /// Specify 0 to set the capacity to the size of the file.</param>
+        /// <param name="access">One of the enumeration values that specifies the type of access allowed
+        /// to the memory-mapped file.<para>This parameter can't be set to <c>Write</c>.</para></param>
+        /// <param name="inheritability">One of the enumeration values that specifies whether a handle
+        /// to the memory-mapped file can be inherited by a child process. The default is <c>None</c>.</param>
+        /// <param name="leaveOpen">A value that indicates whether to close the source file handle when
+        /// the <see cref="MemoryMappedFile"/> is disposed.</param>
+        /// <returns>A memory-mapped file that has the specified characteristics.</returns>
+        /// <exception cref="ArgumentException">
+        /// <para><paramref name="mapName"/> is <see langword="null" /> or an empty string.</para>
+        /// <para>-or-
+        /// <paramref name="capacity"/> and the length of the file are zero.</para>
+        /// <para>-or-
+        /// <paramref name="capacity"/> is set to Write or Write enumeration value, which is not allowed.</para>
+        /// <para>-or-
+        /// access is set to Read and <paramref name="capacity"/> is larger than the length of the file.</para>
+        /// </exception>
+        /// <exception cref="ArgumentNullException"><paramref name="fileHandle"/> is <see langword="null" />.</exception>
+        /// <exception cref="ArgumentOutOfRangeException">
+        /// <paramref name="capacity"/> is less than zero.
+        /// <para>-or-
+        /// <paramref name="capacity"/> is less than the file size.</para>
+        /// <para>-or-
+        /// <paramref name="access"/> is not a valid <see cref="MemoryMappedFileAccess"/> enumeration value.</para>
+        /// <para>-or-
+        /// <paramref name="inheritability"/> is not a valid <see cref="HandleInheritability"/> enumeration value.</para>
+        /// </exception>


The comment needs some minor tweaking:

when referring to existing types we need to use <see cref="$typeName"/> everywhere, so IDE gives users the possibility to click the link and go to the type.

same goes for enum values

<paramref name="capacity"/> is set to Write was incorrect, I assume we need <paramref name="access"/> is set to <see cref="MemoryMappedFileAccess.Write"/>

BTW @carlossanlop @gewarren do we a doc that describes XML comments best practices?

Suggested change

/// <summary>

/// Creates a memory-mapped file from an existing file using a SafeFileHandle,

/// and the specified access mode, name, inheritability, and capacity.

/// </summary>

/// <param name="fileHandle">The SafeFileHandle to the existing file. Caller is

/// responsible for disposing <paramref name="fileHandle"/> when <paramref name="leaveOpen"/> is <see langword="true" /> (otherwise,

/// automatically disposed by the <see cref="MemoryMappedFile"/>). </param>

/// <param name="mapName">A name to assign to the memory-mapped file, or <see langword="null" /> for a

/// <see cref="MemoryMappedFile"/> that you do not intend to share across processes.</param>

/// <param name="capacity">The maximum size, in bytes, to allocate to the memory-mapped file.

/// Specify 0 to set the capacity to the size of the file.</param>

/// <param name="access">One of the enumeration values that specifies the type of access allowed

/// to the memory-mapped file.<para>This parameter can't be set to <c>Write</c>.</para></param>

/// <param name="inheritability">One of the enumeration values that specifies whether a handle

/// to the memory-mapped file can be inherited by a child process. The default is <c>None</c>.</param>

/// <param name="leaveOpen">A value that indicates whether to close the source file handle when

/// the <see cref="MemoryMappedFile"/> is disposed.</param>

/// <returns>A memory-mapped file that has the specified characteristics.</returns>

/// <exception cref="ArgumentException">

/// <para><paramref name="mapName"/> is <see langword="null" /> or an empty string.</para>

/// <para>-or-

/// <paramref name="capacity"/> and the length of the file are zero.</para>

/// <para>-or-

/// <paramref name="capacity"/> is set to Write or Write enumeration value, which is not allowed.</para>

/// <para>-or-

/// access is set to Read and <paramref name="capacity"/> is larger than the length of the file.</para>

/// </exception>

/// <exception cref="ArgumentNullException"><paramref name="fileHandle"/> is <see langword="null" />.</exception>

/// <exception cref="ArgumentOutOfRangeException">

/// <paramref name="capacity"/> is less than zero.

/// <para>-or-

/// <paramref name="capacity"/> is less than the file size.</para>

/// <para>-or-

/// <paramref name="access"/> is not a valid <see cref="MemoryMappedFileAccess"/> enumeration value.</para>

/// <para>-or-

/// <paramref name="inheritability"/> is not a valid <see cref="HandleInheritability"/> enumeration value.</para>

/// </exception>

/// <summary>

/// Creates a memory-mapped file from an existing file using a <see cref="SafeFileHandle"/>,

/// and the specified access mode, name, inheritability, and capacity.

/// </summary>

/// <param name="fileHandle">The <see cref="SafeFileHandle"/> to the existing file. Caller is

/// responsible for disposing <paramref name="fileHandle"/> when <paramref name="leaveOpen"/> is <see langword="true" /> (otherwise,

/// automatically disposed by the <see cref="MemoryMappedFile"/>). </param>

/// <param name="mapName">A name to assign to the memory-mapped file, or <see langword="null" /> for a

/// <see cref="MemoryMappedFile"/> that you do not intend to share across processes.</param>

/// <param name="capacity">The maximum size, in bytes, to allocate to the memory-mapped file.

/// Specify 0 to set the capacity to the size of the file.</param>

/// <param name="access">One of the enumeration values that specifies the type of access allowed

/// to the memory-mapped file.

/// <para>This parameter can't be set to <see cref="MemoryMappedFileAccess.Write"/></para></param>

/// <param name="inheritability">One of the enumeration values that specifies whether a handle

/// to the memory-mapped file can be inherited by a child process. The default is <see cref="HandleInheritability.None"/>.</param>

/// <param name="leaveOpen">A value that indicates whether to close the source file handle when

/// the <see cref="MemoryMappedFile"/> is disposed.</param>

/// <returns>A memory-mapped file that has the specified characteristics.</returns>

/// <exception cref="ArgumentException">

/// <para><paramref name="mapName"/> is <see langword="null" /> or an empty string.</para>

/// <para>-or-</para>

/// <para><paramref name="capacity"/> and the length of the file are zero.</para>

/// <para>-or-</para>

/// <para><paramref name="access"/> is set to <see cref="MemoryMappedFileAccess.Write"/>, which is not allowed.</para>

/// <para>-or-</para>

/// <para><paramref name="access"/> is set to <see cref="MemoryMappedFileAccess.Read"/> and <paramref name="capacity"/> is larger than the length of the file.</para>

/// </exception>

/// <exception cref="ArgumentNullException"><paramref name="fileHandle"/> is <see langword="null" />.</exception>

/// <exception cref="ArgumentOutOfRangeException">

/// <para><paramref name="capacity"/> is less than zero.</para>

/// <para>-or-</para>

/// <para><paramref name="capacity"/> is less than the file size.</para>

/// <para>-or-</para>

/// <para><paramref name="access"/> is not a valid <see cref="MemoryMappedFileAccess"/> enumeration value.</para>

/// <para>-or-</para>

/// <para><paramref name="inheritability"/> is not a valid <see cref="HandleInheritability"/> enumeration value.</para>

/// </exception>

@adamsitnik We do have a guide, but I believe it's internal only: https://review.learn.microsoft.com/en-us/help/contribute-ref/how-to-write-net-docs?branch=main

@gewarren this guide is great, we should open source it!

@adamsitnik Just published the /// guide to the public contributor guide!
https://learn.microsoft.com/contribute/dotnet/api-documentation

adamsitnik · 2023-03-21T08:32:25Z

src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs

+        [Fact]
+        public void InvalidArguments_SafeFileHandle()
+        {
+            // null is an invalid stream


nit: minor copy-paste error

Suggested change

// null is an invalid stream

// null is an invalid handle

adamsitnik · 2023-03-21T08:42:00Z

src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs

+            using (FileStream fs = File.Open(file.Path, FileMode.Open))
+            {
+                SafeFileHandle fileHandle = fs.SafeFileHandle;


This code is 100% correct, but we can improve it by not creating the FileStream and then using it's SafeFileHandle, but rather using the File.OpenHandle and work with handle directly.

The reason why I am asking for this change is that when we introduce new APIs we often don't provide docs with examples for them, so some users use our tests as examples. And if they want to use the new SafeFileHandle based APIs, they should not be creating FileStream up-front. FileStream is more expensive in terms of allocations (it's simply bigger and might allocate it's own buffer), moreover accessing FileStream.SafeFileHandle is performing a sys-call, which is expensive:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs

Lines 96 to 105 in 9eabb99

internal sealed override SafeFileHandle SafeFileHandle

{

get

{

if (CanSeek)

{

// Update the file offset before exposing it since it's possible that

// in memory position is out-of-sync with the actual file position.

FileStreamHelpers.Seek(_fileHandle, _filePosition, SeekOrigin.Begin);

}

Suggested change

using (FileStream fs = File.Open(file.Path, FileMode.Open))

{

SafeFileHandle fileHandle = fs.SafeFileHandle;

using (SafeFileHandle fileHandle = File.OpenHandle(file.Path, FileMode.Open))

{

…79187

adamsitnik

LGTM, thank you very much for your contribution @amalabey !

amalabey added 7 commits March 7, 2023 22:59

Add CreateFromFile overload with SafeFileHandle

0dbd989

The overload uses the handle the perform validations instead of instantiating a FileStream and calling the FileStream overload, because the FileStream instantiation appears to be heavy on allocations.

Add invalid capacity tests

7c7c906

Refactor test name to align with existing

dc96bfa

Increase test coverage for SafeFileHandle

2fb6b01

Merge branch 'dotnet:main' into issue-79187

ec9b0e4

Add API docs for the new overload

a6204ff

Remove unnecessary line

a1e9215

dotnet-issue-labeler bot added area-System.IO new-api-needs-documentation labels Mar 8, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 8, 2023

adamsitnik reviewed Mar 15, 2023

View reviewed changes

amalabey added 4 commits March 16, 2023 10:27

Merge remote-tracking branch 'upstream/main' into issue-79187

bcac9ab

Fix xml doc issues

91a50cc

Fix paramref and param mistakes

d1d8d53

Separate and reuse validation logic

c52a66c

build-analysis bot mentioned this pull request Mar 16, 2023

Crashes when initializing System.Net.Security.Native #83540

Closed

kunalspathak mentioned this pull request Mar 17, 2023

Segregate the register iterations based on intType/floatType #83109

Closed

2 tasks

amalabey added 2 commits March 17, 2023 22:12

Tag failing test with the related active issue

1e2cb2a

Merge remote-tracking branch 'upstream/main' into issue-79187

9eabb99

amalabey requested a review from adamsitnik March 18, 2023 03:35

adamsitnik reviewed Mar 21, 2023

View reviewed changes

amalabey added 5 commits March 22, 2023 21:03

Fix xmldoc formatting issues

41410d1

Fix copy paste error

e582381

Use fileHandle from File.OpenHandle

a7c653e

Merge remote-tracking branch 'upstream/main' into issue-79187

834bc9e

Merge branch 'issue-79187' of github.com:amalabey/runtime into issue-…

b6faed7

…79187

adamsitnik approved these changes Mar 22, 2023

View reviewed changes

adamsitnik merged commit 6bcd4aa into dotnet:main Mar 22, 2023

amalabey deleted the issue-79187 branch March 22, 2023 22:00

ghost locked as resolved and limited conversation to collaborators Apr 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #79187 Add MemoryMappedFile.CreateFromFile(SafeFileHandle) overload #83134

Fixes #79187 Add MemoryMappedFile.CreateFromFile(SafeFileHandle) overload #83134

amalabey commented Mar 8, 2023

dotnet-issue-labeler bot commented Mar 8, 2023

ghost commented Mar 8, 2023

amalabey commented Mar 8, 2023

adamsitnik left a comment

adamsitnik Mar 15, 2023

amalabey Mar 18, 2023

adamsitnik Mar 15, 2023

amalabey Mar 15, 2023

amalabey Mar 18, 2023

amalabey commented Mar 18, 2023

adamsitnik left a comment

adamsitnik Mar 21, 2023

gewarren Mar 21, 2023

adamsitnik Mar 22, 2023

gewarren Jun 30, 2023

adamsitnik Mar 21, 2023

adamsitnik Mar 21, 2023

adamsitnik left a comment

	internal sealed override SafeFileHandle SafeFileHandle
	{
	get
	{
	if (CanSeek)
	{
	// Update the file offset before exposing it since it's possible that
	// in memory position is out-of-sync with the actual file position.
	FileStreamHelpers.Seek(_fileHandle, _filePosition, SeekOrigin.Begin);
	}

Fixes #79187 Add MemoryMappedFile.CreateFromFile(SafeFileHandle) overload #83134

Fixes #79187 Add MemoryMappedFile.CreateFromFile(SafeFileHandle) overload #83134

Conversation

amalabey commented Mar 8, 2023

dotnet-issue-labeler bot commented Mar 8, 2023

ghost commented Mar 8, 2023

amalabey commented Mar 8, 2023

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amalabey commented Mar 18, 2023

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment