Skip to content
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.

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

Ignore IOException in PollingFileChangeToken #57376

Merged
3 commits merged into from
Aug 14, 2021
Merged

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Aug 13, 2021

Contributes to #57221, ignores the exception to unblock clean CI.
I've also split throw calls to one per stat/lstat to identify which is the one throwing EINVAL.

@jozkee jozkee added this to the 6.0.0 milestone Aug 13, 2021
@jozkee jozkee self-assigned this Aug 13, 2021
@ghost
Copy link

ghost commented Aug 13, 2021

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

Issue Details

Contributes to #57221, ignores the exception to unblock clean CI.
I've also split throw calls to one per stat/lstat to identify which is the one throwing EINVAL.

Author: Jozkee
Assignees: Jozkee
Labels:

area-System.IO

Milestone: 6.0.0

@danmoseley
Copy link
Member

this is fine (with fix I noted above) but I wonder whether it would be helpful (in another PR) to log the path for more exception types in GetExceptionForIoErrno. Right now although you pass the path it drops the path for several error codes, including the EINVAL in this case

return new IOException(errorInfo.GetErrorMessage(), errorInfo.RawErrno);

You could potentially improve that to include the path. It would mean adding a new string -- look for ArgumentOutOfRange_FileLengthTooBig to find the 11 resx files that would need it. The string would look like "Error accessing path '{0}'. {1}" and the code would be

    internal static Exception GetIOException(Interop.ErrorInfo errorInfo)
    {
                return path != null ?
                    new IOException(SR.Format(SR.IO_YourNewString, path, errorInfo.GetErrorMessage()), errorInfo.RawErrno) :
                    new IOException(errorInfo.GetErrorMessage(), errorInfo.RawErrno);
    }

I think that would be a reasonable product change that would also potentially help with this bug.

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

lgtm with the note

@danmoseley
Copy link
Member

The tests are failing because somewhere we passed -1 to the constructor of ErrorInfo.

@jozkee
Copy link
Member Author

jozkee commented Aug 13, 2021

The tests are failing because somewhere we passed -1 to the constructor of ErrorInfo.

Addressed in 6d09715.

@ghost
Copy link

ghost commented Aug 14, 2021

Hello @jozkee!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 3374616 into dotnet:main Aug 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2021
@jozkee jozkee deleted the polling-ioexception branch April 16, 2024 15:50
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants