Skip to content

Auto port System.Threading ACL comments #4147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 27, 2020
Merged

Auto port System.Threading ACL comments #4147

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d0d01b
Auto port System.Threading ACL comments
carlossanlop Apr 21, 2020
959e060
Apply suggestions from code review
carlossanlop Apr 22, 2020
e631482
Apply suggestions from code review
carlossanlop Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions xml/System.Threading/EventWaitHandleAcl.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Type Name="EventWaitHandleAcl" FullName="System.Threading.EventWaitHandleAcl">
<Type Name="EventWaitHandleAcl" FullName="System.Threading.EventWaitHandleAcl">
<TypeSignature Language="C#" Value="public static class EventWaitHandleAcl" />
<TypeSignature Language="ILAsm" Value=".class public auto ansi abstract sealed beforefieldinit EventWaitHandleAcl extends System.Object" />
<TypeSignature Language="DocId" Value="T:System.Threading.EventWaitHandleAcl" />
Expand Down Expand Up @@ -41,14 +41,28 @@
<Parameter Name="eventSecurity" Type="System.Security.AccessControl.EventWaitHandleSecurity" />
</Parameters>
<Docs>
<param name="initialState">To be added.</param>
<param name="mode">To be added.</param>
<param name="name">To be added.</param>
<param name="createdNew">To be added.</param>
<param name="eventSecurity">To be added.</param>
<summary>To be added.</summary>
<returns>To be added.</returns>
<remarks>To be added.</remarks>
<param name="initialState">
<see langword="true" /> to set the initial state to signaled if the named event is created as a result of this call; <see langword="false" /> to set it to non-signaled.</param>
<param name="mode">One of the enum values that determines whether the event resets automatically or manually.</param>
<param name="name">The name, if the event is a system-wide synchronization event; otherwise, <see langword="null" /> or an empty string.</param>
<param name="createdNew">When this method returns, this argument is always set to <see langword="true" /> if a local event is created; that is, when <paramref name="name" /> is <see langword="null" /> or <see cref="F:System.String.Empty" />. If <paramref name="name" /> has a valid, non-empty value, this argument is set to <see langword="true" /> when the system event is created, or it is set to <see langword="false" /> if an existing system event is found with that name. This parameter is passed uninitialized.</param>
<param name="eventSecurity">The optional Windows access control security to apply.</param>
<summary>Gets or creates an <see cref="T:System.Threading.EventWaitHandle" /> instance, allowing a <see cref="T:System.Security.AccessControl.EventWaitHandleSecurity" /> instance to be optionally specified to set it during the event creation.</summary>
<returns>An object that represents a system event wait handle, if named, or a local event wait handle, if nameless.</returns>
<remarks>
<format type="text/markdown"><![CDATA[
## Remarks
If a `name` is passed and the system event already exists, the existing event is returned. If `name` is `null` or <xref:System.String.Empty>, a new local event is always created.
]]></format>
</remarks>
<exception cref="T:System.ArgumentNullException">.NET Framework only: The <paramref name="name" /> length is beyond MAX_PATH (260 characters).</exception>
<exception cref="T:System.ArgumentOutOfRangeException">The <paramref name="mode" /> enum value was out of legal range.</exception>
Copy link
Contributor

@kouvel kouvel Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see an ArgumentException thrown by .NET Framework, ArgumentOutOfRangeException would make sense in this case but I'm wondering now if I'm seeing different docs on which this port was based (I'm currently looking at .NET Framework docs for the equivalent APIs).

Copy link
Contributor Author

@carlossanlop carlossanlop Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.NET Core: https://github.com/dotnet/runtime/blob/8dc13298a89d50e64f24d0e755675bb01a33a399/src/libraries/System.Threading.AccessControl/src/System/Threading/EventWaitHandleAcl.cs#L35-L38

You're right, I should indicate another exception for Framework. I'll add it to the suggestion from the previous comment. https://referencesource.microsoft.com/#mscorlib/system/threading/eventwaithandle.cs,136

Copy link
Contributor

@kouvel kouvel Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm there are many inconsistencies between in-code docs, tests, and .NET Framework docs. Filed dotnet/runtime#35519 to fix those, I think following the XML docs is good for this PR.

<exception cref="T:System.IO.DirectoryNotFoundException">Could not find a part of the path specified in <paramref name="name" />.</exception>
Copy link
Contributor

@kouvel kouvel Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this exception being raised by this API, the name parameter has nothing to do with directories on Windows, and on Unixes it's an implementation detail and probably should not result in such an exception

Copy link
Contributor Author

@carlossanlop carlossanlop Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we reach here: https://github.com/dotnet/runtime/blob/8dc13298a89d50e64f24d0e755675bb01a33a399/src/libraries/System.Threading.AccessControl/src/System/Threading/EventWaitHandleAcl.cs#L82

We throw here: https://github.com/dotnet/runtime/blob/25d316cefafc4c3884f38acf7bdef971d2b9fe4b/src/libraries/Common/src/System/IO/Win32Marshal.cs#L39

Unit test verifying this exception added here: https://github.com/dotnet/runtime/blob/25d316cefafc4c3884f38acf7bdef971d2b9fe4b/src/libraries/System.Threading.AccessControl/tests/EventWaitHandleAclTests.cs#L70-L84

Copy link
Contributor

@kouvel kouvel Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see thanks, I didn't know about that. If it can throw this exception for EventWaitHandle then it would throw in similar cases for the other types as well. This is fine for now, I'll look into fixing issues, filed issue dotnet/runtime#35518.

<exception cref="T:System.Threading.WaitHandleCannotBeOpenedException">A system-wide synchronization event with the provided <paramref name="name" /> was not found.
-or- An <see cref="T:System.Threading.EventWaitHandle" /> with system-wide name <paramref name="name" /> cannot be created. An <see cref="T:System.Threading.EventWaitHandle" /> of a different type might have the same name.</exception>
</Docs>
</Member>
</Members>
Expand Down
17 changes: 10 additions & 7 deletions xml/System.Threading/MutexAcl.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Type Name="MutexAcl" FullName="System.Threading.MutexAcl">
<Type Name="MutexAcl" FullName="System.Threading.MutexAcl">
<TypeSignature Language="C#" Value="public static class MutexAcl" />
<TypeSignature Language="ILAsm" Value=".class public auto ansi abstract sealed beforefieldinit MutexAcl extends System.Object" />
<TypeSignature Language="DocId" Value="T:System.Threading.MutexAcl" />
Expand Down Expand Up @@ -40,13 +40,16 @@
<Parameter Name="mutexSecurity" Type="System.Security.AccessControl.MutexSecurity" />
</Parameters>
<Docs>
<param name="initiallyOwned">To be added.</param>
<param name="name">To be added.</param>
<param name="createdNew">To be added.</param>
<param name="mutexSecurity">To be added.</param>
<summary>To be added.</summary>
<returns>To be added.</returns>
<param name="initiallyOwned">
<see langword="true" /> to give the calling thread initial ownership of the named system mutex if the named system mutex is created as a result of this call; otherwise, <see langword="false" />.</param>
<param name="name">The optional name of the system mutex. If this argument is set to <see langword="null" /> or <see cref="F:System.String.Empty" />, a local mutex is created.</param>
<param name="createdNew">When this method returns, this argument is always set to <see langword="true" /> if a local mutex is created; that is, when <paramref name="name" /> is <see langword="null" /> or <see cref="F:System.String.Empty" />. If <paramref name="name" /> has a valid non-empty value, this argument is set to <see langword="true" /> when the system mutex is created, or it is set to <see langword="false" /> if an existing system mutex is found with that name. This parameter is passed uninitialized.</param>
<param name="mutexSecurity">The optional mutex access control security to apply.</param>
<summary>Gets or creates <see cref="T:System.Threading.Mutex" /> instance, allowing a <see cref="T:System.Security.AccessControl.MutexSecurity" /> to be optionally specified to set it during the mutex creation.</summary>
<returns>An object that represents a system mutex, if named, or a local mutex, if nameless.</returns>
<remarks>To be added.</remarks>
<exception cref="T:System.ArgumentException">.NET Framework only: The length of the name exceeds the maximum limit.</exception>
<exception cref="T:System.Threading.WaitHandleCannotBeOpenedException">A mutex handle with system-wide <paramref name="name" /> cannot be created. A mutex handle of a different type might have the same name.</exception>
</Docs>
</Member>
</Members>
Expand Down
24 changes: 16 additions & 8 deletions xml/System.Threading/SemaphoreAcl.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Type Name="SemaphoreAcl" FullName="System.Threading.SemaphoreAcl">
<Type Name="SemaphoreAcl" FullName="System.Threading.SemaphoreAcl">
<TypeSignature Language="C#" Value="public static class SemaphoreAcl" />
<TypeSignature Language="ILAsm" Value=".class public auto ansi abstract sealed beforefieldinit SemaphoreAcl extends System.Object" />
<TypeSignature Language="DocId" Value="T:System.Threading.SemaphoreAcl" />
Expand Down Expand Up @@ -41,14 +41,22 @@
<Parameter Name="semaphoreSecurity" Type="System.Security.AccessControl.SemaphoreSecurity" />
</Parameters>
<Docs>
<param name="initialCount">To be added.</param>
<param name="maximumCount">To be added.</param>
<param name="name">To be added.</param>
<param name="createdNew">To be added.</param>
<param name="semaphoreSecurity">To be added.</param>
<summary>To be added.</summary>
<returns>To be added.</returns>
<param name="initialCount">The initial number of requests for the semaphore that can be satisfied concurrently.</param>
<param name="maximumCount">The maximum number of requests for the semaphore that can be satisfied concurrently.</param>
<param name="name">Optional argument to create a system semaphore. Set to <see langword="null" /> or <see cref="F:System.String.Empty" /> to create a local semaphore.</param>
<param name="createdNew">When this method returns, this argument is always set to <see langword="true" /> if a local semaphore is created; that is, when <paramref name="name" /> is <see langword="null" /> or <see cref="F:System.String.Empty" />. If <paramref name="name" /> has a valid, non-empty value, this argument is set to <see langword="true" /> when the system semaphore is created, or it is set to <see langword="false" /> if an existing system semaphore is found with that name. This parameter is passed uninitialized.</param>
<param name="semaphoreSecurity">The optional semaphore access control security to apply.</param>
<summary>Gets or creates an <see cref="T:System.Threading.Semaphore" /> instance, allowing a <see cref="T:System.Security.AccessControl.SemaphoreSecurity" /> instance to be optionally specified to set it during the event creation.</summary>
<returns>An object that represents a system semaphore, if named, or a local semaphore, if nameless.</returns>
<remarks>To be added.</remarks>
<exception cref="T:System.ArgumentOutOfRangeException">
<paramref name="initialCount" /> is a negative number.
-or-

<paramref name="maximumCount" /> is not a positive number.</exception>
<exception cref="T:System.ArgumentException">
<paramref name="initialCount" /> is greater than <paramref name="maximumCount" />.</exception>
<exception cref="T:System.Threading.WaitHandleCannotBeOpenedException">A semaphore handle with the system-wide name '<paramref name="name" />' cannot be created. A semaphore handle of a different type might have the same name.</exception>
</Docs>
</Member>
</Members>
Expand Down