Add a delegate for OnDirectoryFinished() and others

[iSazonov]

Background and motivation

Developers need a way to handle events while recursing directories with FileSystemEnumerable. The FileSystemEnumerator API already gives the ability to do this by exposing ContinueOnError(int), OnDirectoryFinished(ROS<char>), ShouldIncludeEntry(FilesystemEntry), and ShouldRecurseIntoEntry(FileSystemEntry). The last two methods can be easily used with the FileSystemEnumerable API as it allows you set delegates for them. This proposal is about completing the scenario by adding two more delegate properties for the remaining two.

Use case description:

In PowerShell Core repo we are trying to migrate to FileSystemEnumerable.
We need to implement error handling so that continue processing on an error and logging the error.
Also for consistency it would be great to get OnDirectoryFinishedDelegate.

Proposed API

public class FileSystemEnumerable<TResult> : IEnumerable<TResult>
{
    public FindPredicate? ShouldIncludePredicate { get; set; } // Existing API
    public FindPredicate? ShouldRecursePredicate { get; set; } // Existing API

    public delegate void OnDirectoryFinishedDelegate(ReadOnlySpan<char> directory); // New API
    public OnDirectoryFinishedDelegate? OnDirectoryFinishedAction { get; set; } // New API

    public delegate bool ContinueOnErrorPredicate(int error); // New API
    public ContinueOnErrorPredicate? ShouldContinueOnErrorPredicate { get; set; } // New API
}

API usage

using System.IO.Enumeration;

string directory = "...";
var context = ... // PowerShell context

var files = new FileSystemEnumerable<string>(
    directory,
    // Yes, it is weird but we should write right error for root directory too.
    (ref FileSystemEntry entry) => entry.OriginalRootDirectory.Length > entry.Directory.Length ? entry.FileName.ToString() : Path.Join(entry.Directory.Slice(entry.OriginalRootDirectory.Length), entry.FileName),
    options)
{
    ShouldContinueOnErrorPredicate = (int error) =>
    {
        if (error == 5) // ERROR_ACCESS_DENIED
        {
            // Log the error...
        }

        if (error == 3) // ERROR_PATH_NOT_FOUND
        {
            return false;
        }

        return true;
    }
};

Alternative Designs: define FileSystemErrorInfo to abstract the OS native error (this differs from FileSystemEnumerator<T>.ContinueOnError(int)).

public class FileSystemEnumerable<TResult> : IEnumerable<TResult>
{
    public FindPredicate? ShouldIncludePredicate { get; set; } // Existing API
    public FindPredicate? ShouldRecursePredicate { get; set; } // Existing API

    public delegate void OnDirectoryFinishedDelegate(ReadOnlySpan<char> directory); // New API
    public OnDirectoryFinishedDelegate? OnDirectoryFinishedAction { get; set; } // New API


    public delegate bool ContinueOnErrorPredicate(FileSystemErrorInfo errorInfo); // New API
    public ContinueOnErrorPredicate? ShouldContinueOnErrorPredicate { get; set; } // New API
}

public readonly ref struct FileSystemErrorInfo
{
    // internal FileSystemErrorInfo(int errno, string path);
    public int Error { get; }; // 'errno' is converted on Unix with `internal struct ErrorInfo` to platform-agnostic Error
    public string Path { get; };
    public Exception GetException(); // Gets platform-agnostic exception for the error
    public string GetErrorMessage(); // Optional for me but could be useful due to absence of extra allocations with GetException()
}

public unsafe abstract partial class FileSystemEnumerator<TResult> : CriticalFinalizerObject, IEnumerator<TResult>
{
    [Obsolete("The method is deprecated. Use ContinueOnError(FileSystemErrorInfo errorInfo) instead.")] // New API
    protected virtual bool ContinueOnError(int error); // Deprecate
    protected virtual bool ContinueOnError(FileSystemErrorInfo errorInfo);  // New API
}

Remarkable questions

• Should we use events?
• Should OnDirectoryFinishedDelegate(ROS<char>) should be OnDirectoryFinishedDelegate(FilesystemEntry)
  This would differ from what the Enumerator type exposes; implementation-wise we would need to create an extra FileSystemEntry that represents the root directory (could impact perf?).

[JeremyKuhne]

This is not the first time I've been asked for this and it is pretty straight-forward, I think we should add this for 5.0.

[iSazonov]

For full consistency I'd add a delegate for ContinueOnError() too.

[danmoseley]

@iSazonov How would the "depth limit" scenario work with these predicates? In the implementation of ShouldRecursePredicate would it examine the FileSystemEntry and then calculate the depth vs the start of the recursion (perhaps with Path.GetRelativePath and counting the double dots?)

As with any API proposal it might be useful to write out what the user code would look like.

[iSazonov]

@danmosemsft You can see draft code in PowerShell repo PowerShell/PowerShell#12834. The PR demonstrates well a real scenario of using the API.

[danmoseley]

@iSazonov to make the API review meetings efficient we ask that all the supporting info be in the proposal (ideally in the top post). For API proposals this ideally includes small usage examples to help demo how the API would be used

[iSazonov]

@danmosemsft I added a code snipped from the PowerShell PR.

[danmoseley]

Thanks, that makes it clear.

[terrajobst]

Video

• It seems OnDirectoryFinishedDelegate should include the FileSystemEntry entry
• Should ContinueOnErrorPredicate provide some context? It seems one would want the container and the name the child to do anything useful for logging. If we see potential for more state, we may want to allow a struct, such as FileSystemError instead.
• @carlossanlop you may want to sync with @JeremyKuhne on how to evolve this API

namespace System.IO.Enumeration
{
    public partial class FileSystemEnumerable<TResult> : IEnumerable<TResult>
    {
        public delegate void OnDirectoryFinishedDelegate(ref FileSystemEntry entry);
        public delegate bool ContinueOnErrorPredicate(ref FileSystemEntry parent, ReadOnlySpan<char> entry, int error);

        public OnDirectoryFinishedDelegate? OnDirectoryFinishedAction { get; set; }
        public ContinueOnErrorPredicate? ShouldContinueOnErrorPredicate { get; set; }
     }
}

[iSazonov]

It seems OnDirectoryFinishedDelegate should include the FileSystemEntry entry

OnDirectoryFinished is defined as

runtime/src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.cs

Line 63 in 6072e4d

protected virtual void OnDirectoryFinished(ReadOnlySpan<char> directory) { }

and my proposal follows this. If MSFT team want enhance this - I agree.

Should ContinueOnErrorPredicate provide some context?

ContinueOnError is defined as

runtime/src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.cs

Line 70 in 6072e4d

protected virtual bool ContinueOnError(int error) => false;

and my proposal follows this. If MSFT team want enhance this - I agree.

[iSazonov]

Can we continue this work? I still hope to use this in the next PowerShell milestone (LTS!).

I have updated the proposal based on previous discussion and also added a new option to EnumerationOptions to support recursion depth limit.

The current implementation uses a queue to process subdirectories. This allows us to implement the recursion depth limit in the simplest way so I don't need to invent tricky code in PowerShell. I could try to pull a PR and implement the part as only the proposal will be approved.

[carlossanlop]

I have updated the proposal based on previous discussion and also added a new option to EnumerationOptions to support recursion depth limit.

Thank you, @iSazonov.

Should ContinueOnErrorPredicate provide some context? It seems one would want the container and the name the child to do anything useful for logging. If we see potential for more state, we may want to allow a struct, such as FileSystemError instead.

The parameters of ContinueOnErrorPredicate were questioned in the video. The request is to consider thinking of potential future expansions of the error context information, and make sure to include the container of the filesystem item that failed. This could be achieved by adding a struct which would could hold all the relevant error information. For example, you can see that EnumerationOptions was created as a class, so that it can be expanded easily without having to modify the methods that consume it as parameter.

Another thing that was mentioned is that we need to make sure the reported error makes sense across platforms. Windows does not throw the same errors as Linux, but we need to reflect that in our APIs in an OS independent way.

For API proposals this ideally includes small usage examples to help demo how the API would be used

Please don't forget to add new code usage examples to help understand how the modified API structure ties up together.

cc @Jozkee

[iSazonov]

@carlossanlop Thanks for clarification! My today priority is the recursion depth limit, it blocks me. The recursion depth limit API proposal doesn't seem to correlate with other proposed APIs so maybe move it in new issue to move forward faster?

[carlossanlop]

If you don't think they depend will each other, then sure, then I don't mind if we address that API separately.

But for simplicity and clarity, since the other APIs are not ready yet, can you please move the MaxDepthRecursion API to a separate API Proposal issue?

[iSazonov]

@carlossanlop I put new proposal to #43748.

[iSazonov]

    public delegate bool ContinueOnErrorPredicate(ref FileSystemEntry parent, ReadOnlySpan<char> entry, int error);

I have a concern about parent parameter. It seems it will be difficult to implement at subdirectory dequeuing - current implementation does not track parent entry/path and adding this will decrease performance significantly.

As for generalization error reporting, have we any samples of such maooings in the repo?
We could create an exception object like UnauthorizedAccessException/IOException and its.

[Jozkee]

@iSazonov Can we remove MaxDepthRecursion from this proposal since you already created a separate one for it?

[iSazonov]

@Jozkee Done.

[iSazonov]

It seems this is waiting me :-)

If we look current implementation we can get only two actual entities in error time - (1) native error number, (2) current path (on which the error is raised), and we haven't FileSystemEntry in the time.

So proposal is struct FileSystemErrorInfo to wrap these two values. Also we would want to get an OS independent result. Again, current implementation already throws an OS independent exception generated by internal method Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(result), _currentPath, isDirectory: true); on Unix and Win32Marshal.GetExceptionForWin32Error(error, _currentPath). So we can expose the methods in FileSystemErrorInfo with GetException(). This is great to have in PowerShell where we can not only throw but also write a warning instead of the throw. (Ex.: if we do dir -Recuse, PowerShell writes warning messages to user on access denied errors and continue enumeration.

namespace System.IO.Enumeration
{ 
    public class FileSystemEnumerable<TResult> : IEnumerable<TResult>
    {
        public FindPredicate? ShouldIncludePredicate { get; set; } // Existing API
        public FindPredicate? ShouldRecursePredicate { get; set; } // Existing API

        public delegate void OnDirectoryFinishedDelegate(ReadOnlySpan<char> directory); // New API
        public OnDirectoryFinishedDelegate? OnDirectoryFinishedAction { get; set; } // New API


        public delegate bool ContinueOnErrorPredicate(FileSystemErrorInfo errorInfo); // New API
        public ContinueOnErrorPredicate? ShouldContinueOnErrorPredicate { get; set; } // New API
    }

    public readonly ref struct FileSystemErrorInfo
    {
        // internal FileSystemErrorInfo(int errno, string path);
        public int Error { get; }; // 'errno' is converted on Unix with `internal struct ErrorInfo` to platform-agnostic Error
        public string Path { get; };
        public Exception GetException(); // Gets platform-agnostic exception for the error
        public string GetErrorMessage(); // Optional for me but could be useful due to absence of extra allocations with GetException()
    }

    public unsafe abstract partial class FileSystemEnumerator<TResult> : CriticalFinalizerObject, IEnumerator<TResult>
    {
        [Obsolete("The method is deprecated. Use ContinueOnError(FileSystemErrorInfo errorInfo) instead.")] // New API
        protected virtual bool ContinueOnError(int error); // Deprecate
        protected virtual bool ContinueOnError(FileSystemErrorInfo errorInfo);  // New API
    }
}

[iSazonov]

@danmoseley @Jozkee Friendly ping.

[iSazonov]

I'd happy to implement this for PowerShell.

[Jozkee]

For OnDirectoryFinishedDelegate(ref FileSystemEntry entry): do we want to add an equivalent API to FileSystemEnumerator and obsolete the current one taking just a ReadOnlySpan?

For bool ContinueOnErrorPredicate(FileSystemErrorInfo errorInfo): can we consider not adding another type and instead re-use FileSystemEntry? If so, I think the API can look like bool ContinueOnErrorPredicate(int error, ref FileSystemEntry entry) I don't see very useful GetException() and GetErrorMessage().

@iSazonov if you are blocked by this, you think you can extend the Enumerator and work with that? e.g:

class MyFileSystmeEnumerator : FileSystemEnumerator<string>
{
    public MyFileSystmeEnumerator(string directory, EnumerationOptions? options = null) : base(directory, options) { }

    protected override string TransformEntry(ref FileSystemEntry entry)
    {
        return entry.ToFullPath();
    }

    protected override void OnDirectoryFinished(ReadOnlySpan<char> directory)
    {
        // Do what you need
    }

    protected override bool ContinueOnError(int error)
    {
        // Do what you need
    }
}

[iSazonov]

For bool ContinueOnErrorPredicate(FileSystemErrorInfo errorInfo): can we consider not adding another type and instead re-use FileSystemEntry? If so, I think the API can look like bool ContinueOnErrorPredicate(int error, ref FileSystemEntry entry) I don't see very useful GetException() and GetErrorMessage().

If we look current implementation there is no FileSystemEntry instance in points where the error is detected. And I don't see needs to create the instance for "parent" path in the error report (user himself can create Directory/FileInfo if needed) - we can easily have parent path but what about error code - it will OS specific but we need OS agnostic.

As for GetException() and GetErrorMessage(), these address previous API review request to have OS agnostic error report since today we get an OS specific Int value. We could remove FileSystemErrorInfo from the API proposal and return Exception which contains the native error code, but we lost parent path since it will be in error message only. So we again come back to previous API review and suggestion to have new (extendable) type for error reporting.

For OnDirectoryFinishedDelegate(ref FileSystemEntry entry): do we want to add an equivalent API to FileSystemEnumerator and obsolete the current one taking just a ReadOnlySpan?

Yes, it makes sense. If we look current implementation again there is not actual FileSystemEntry instance for parentpath so ReadOnlySpan is ok.

if you are blocked by this, you think you can extend the Enumerator and work with that? e.g:

I can not process ContinueOnError(int error) well and as result the issue was opened :-)

[Jozkee]

these address previous API review request to have OS agnostic

That's interesting because it contradicts the current FileSystemEnumerator<TResult>.ContinueOnError(Int32) API which was approved by the same board. Did you get that feedback for this specific proposal? If not, I think this can be an exception and can be discussed live.

The trade-off with handling int error vs FileSystemErrorInfo errorInfo is that people providing ContinueOnErrorPredicates will have to write OS-specific code for the former, and for the latter the API will diverge from the current API of the enumerator, plus we may need to add an API to the enumerator (to be consistent) and obsolete the current one.

@iSazonov I know you originally opened with the purpose of handling Max Depth, which is now supported by EnumerationOptions. Can you provide samples that will be addressed by adding this new set of APIs?

[iSazonov]

That's interesting because it contradicts the current FileSystemEnumerator.ContinueOnError(Int32) API which was approved by the same board. Did you get that feedback for this specific proposal? If not, I think this can be an exception and can be discussed live.

Sorry, it seems I don't understand your question. If I remember right Jeremy made a proposal for file enumeration with a lot of new APIs, some from them (including ContinueOnError) was under "need feedback from community". First feedback (as Jeremy mentioned) was community want delegates for all virtual methods so I added OnDirectoryFinishedDelegate in the proposal.
Second feedback comes from my experience with PowerShell code. As I already described PowerShell writes warning messages on (access denied) errors and continue enumeration. (And I'd happy to avoid OS Specific error code processing.) So I added new ContinueOnErrorPredicate in the proposal.

Can you provide samples that will be addressed by adding this new set of APIs?

In my old PR PowerShell/PowerShell#12834 you can see how weird looks code (notice, I don't OS specific error code processing there for the example simplicity!) with ContinueOnError(int error) - with ContinueOnError(FileSystemErrorInfo errorInfo) we get exactly we need.

[Jozkee]

First feedback (as #1953 (comment)) was community want delegates for all virtual methods so I added OnDirectoryFinishedDelegate in the proposal.

I agree this proposal is about adding delegates for all virtual in the Enumerator and set them as properties in the Enumerable. What I am pointing out is that the virtual method FileSystemEnumerator<TResult>.ContinueOnError(Int32) already sets precedence for receiving the native error code and should consider follow that precedence.

So I would suggest having FileSystemErrorInfo as an alternative design.

In my old PR PowerShell/PowerShell#12834 you can see how weird looks code (notice, I don't OS specific error code processing there for the example simplicity!) with ContinueOnError(int error) - with ContinueOnError(FileSystemErrorInfo errorInfo) we get exactly we need.

Could you please write a minimal sample in here? It would be hard to scroll through your PR while reviewing the proposal.

[iSazonov]

Could you please write a minimal sample in here? It would be hard to scroll through your PR while reviewing the proposal.

Yes, of cause.

using System.IO.Enumeration;

string directory = "...";
var context = ... // PowerShell context

var files = new FileSystemEnumerable<string>(
    directory,
    // Yes, it is weird but we should write right error for root directory too.
    (ref FileSystemEntry entry) => entry.OriginalRootDirectory.Length > entry.Directory.Length ? entry.FileName.ToString() : Path.Join(entry.Directory.Slice(entry.OriginalRootDirectory.Length), entry.FileName),
    options)
{
    ShouldIncludePredicate = (ref FileSystemEntry entry) => FileSystemEntryFilter, // PowerSHell include filter delegate
    ShouldRecursePredicate = (ref FileSystemEntry entry) =>
    {
        if (context.Recurce && context.IsFollowSymlink)
        {
            // Follow symlinks at recursion.
            return entry.Attributes.HasFlag(FileAttributes.ReparsePoint) && InternalSymbolicLinkLinkCodeMethods.IsReparsePointWithTarget(entry.ToFileSystemInfo());
        }

        return context.Recurce;
    },
    ShouldContinueOnErrorPredicate = (FileSystemErrorInfo errorInfo) =>
    {
        var exc = errorInfo.GetException();
        if (exc is UnauthorizedAccessException)
        {
            context.WriteError(new ErrorRecord(exc, "DirUnauthorizedAccessError", ErrorCategory.PermissionDenied, errorInfo.Path));
        }
        else
        {
            context.WriteError(new ErrorRecord(exc, "DirIOError", ErrorCategory.ReadError, errorInfo.Path));
        }

        return true;
    }
};

[bartonjs]

Video

• There's no need for ContinueOnErrorPredicate, it's Func<int, bool>
• OnDirectoryFinishedAction should use System.String, not ReadOnlySpan<char>. And then it becomes Action<string>.
• We added OnDirectoryStartedAction
• We then removed the prefix "On", but left the suffix "Action" to match the existing "-Predicate" properties.
• We aren't sure if we should remove the "Should" prefix in the on-error handler, or if it's helpful.

Action Required:

• We questioned if the error handler predicate should also take the path that failed.
  • And we didn't know what it should look like (Func<string, int, bool>, something complicated with current directory and current candidate as different parameters, etc)

public class FileSystemEnumerable<TResult> : IEnumerable<TResult>
{
    public Action<string>? DirectoryStartedAction { get; set; } // New API
    public Action<string>? DirectoryFinishedAction { get; set; } // New API

    public Func<int, bool>? ShouldContinueOnErrorPredicate { get; set; } // New API
}

[iSazonov]

I agree it is important to pass current context to custom error handler.
Previous proposal contains readonly ref struct FileSystemErrorInfo to pass a context to the custom error handler.
But if we would agree errors in enumaration time are rare we could pass exception instance which contains all information we need and more (like resource string message). This is very convenient and eliminates the need to duplicate non-trivial code Interop.GetExceptionForIoErrno(). Notice, the exception is OS indepentent!
So we could rewrite follow

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs

Lines 69 to 82 in 2f3fcca

	private IntPtr CreateDirectoryHandle(string path, bool ignoreNotFound = false)
	{
	IntPtr handle = Interop.Sys.OpenDir(path);
	if (handle == IntPtr.Zero)
	{
	Interop.ErrorInfo info = Interop.Sys.GetLastErrorInfo();
	if (InternalContinueOnError(info, ignoreNotFound))
	{
	return IntPtr.Zero;
	}
	throw Interop.GetExceptionForIoErrno(info, path, isDirError: true);
	}
	return handle;
	}

with

        private IntPtr CreateDirectoryHandle(string path, bool ignoreNotFound = false)
        {
            IntPtr handle = Interop.Sys.OpenDir(path);
            if (handle == IntPtr.Zero)
            {
                Interop.ErrorInfo info = Interop.Sys.GetLastErrorInfo();
                if (InternalContinueOnError(info, ignoreNotFound))
                {
                    return IntPtr.Zero;
                }

		var exception = Interop.GetExceptionForIoErrno(info, path, isDirError: true)
                if (ContinueOnError(exception))
                {
                    return IntPtr.Zero;
                }
                throw exception;
            }
            return handle;
        }

This is consistent with the way PowerShell works - catch an exception and print a friendly message to the user, i.e. the exception instance is used anyway.
If there are applications where efficiency is critical at this point, they can use the existing ContinueOnError(int error).

Current proposal:

namespace System.IO.Enumeration
{ 
    public class FileSystemEnumerable<TResult> : IEnumerable<TResult>
    {
        public FindPredicate? ShouldIncludePredicate { get; set; } // Existing API
        public FindPredicate? ShouldRecursePredicate { get; set; } // Existing API

        public Action<string>? DirectoryStartedAction { get; set; } // New API
        public Action<string>? DirectoryFinishedAction { get; set; } // New API

        public Func<Exception, bool>? ShouldContinueOnErrorPredicate { get; set; } // New API
    }

    public unsafe abstract partial class FileSystemEnumerator<TResult> : CriticalFinalizerObject, IEnumerator<TResult>
    {
        [Obsolete("The method is deprecated. Use ContinueOnError(FileSystemErrorInfo errorInfo) instead.")] // New API
        protected virtual bool ContinueOnError(int error); // Deprecate or no?
        protected virtual bool ContinueOnError(Exception exception);  // New API
    }
}

[CyberSinh]

I understand that this thread still needs some work before it can be reviewed again.
What exactly is expected from the .NET team?
No feedback has been given on the API suggestion proposed by @iSazonov above.