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.

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

Jump to bottom

PhysicalFileProvider: Avoid NullRef if ResolveLinkTarget returns null #57235

Merged
merged 1 commit into from
Aug 12, 2021
Merged

PhysicalFileProvider: Avoid NullRef if ResolveLinkTarget returns null #57235

merged 1 commit into from
Aug 12, 2021

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Aug 11, 2021

Spotted the following exception in #57229 (helix log)

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.Extensions.FileProviders.Physical.FileSystemInfoHelper.GetFileLinkTargetLastWriteTimeUtc(FileInfo fileInfo) in /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/Internal/FileSystemInfoHelper.cs:line 57
   at Microsoft.Extensions.FileProviders.Physical.PollingFileChangeToken.GetLastWriteTimeUtc() in /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs:line 57
   at Microsoft.Extensions.FileProviders.Physical.PollingFileChangeToken.get_HasChanged() in /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs:line 101
   at Microsoft.Extensions.FileProviders.Physical.PhysicalFilesWatcher.RaiseChangeEvents(Object state) in /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs:line 455
   at System.Threading.TimerQueueTimer.Fire(Boolean isThreadPool) in /_/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs:line 673
   at System.Threading.TimerQueue.FireNextTimers() in /_/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs:line 326

@jozkee jozkee added this to the 6.0.0 milestone Aug 11, 2021
@jozkee jozkee requested a review from carlossanlop August 11, 2021 20:40
@jozkee jozkee self-assigned this Aug 11, 2021
@ghost
Copy link

ghost commented Aug 11, 2021

Tagging subscribers to this area: @maryamariyan, @dotnet/area-extensions-filesystem
See info in area-owners.md if you want to be subscribed.

Issue Details

Spotted the following exception in #57229 (helix log)

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.Extensions.FileProviders.Physical.FileSystemInfoHelper.GetFileLinkTargetLastWriteTimeUtc(FileInfo fileInfo) in /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/Internal/FileSystemInfoHelper.cs:line 57
   at Microsoft.Extensions.FileProviders.Physical.PollingFileChangeToken.GetLastWriteTimeUtc() in /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs:line 57
   at Microsoft.Extensions.FileProviders.Physical.PollingFileChangeToken.get_HasChanged() in /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs:line 101
   at Microsoft.Extensions.FileProviders.Physical.PhysicalFilesWatcher.RaiseChangeEvents(Object state) in /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs:line 455
   at System.Threading.TimerQueueTimer.Fire(Boolean isThreadPool) in /_/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs:line 673
   at System.Threading.TimerQueue.FireNextTimers() in /_/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs:line 326

<table>
  <tr>
    <th align="left">Author:</th>
    <td>Jozkee</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>Jozkee</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`area-Extensions-FileSystem`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>6.0.0</td>
  </tr>
</table>
</details>

carlossanlop
carlossanlop approved these changes Aug 11, 2021
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

LGTM

@danmoseley danmoseley mentioned this pull request Aug 11, 2021
Merged
danmoseley
danmoseley approved these changes Aug 11, 2021
View reviewed changes
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.

Wondering whether this is unit testable. (Although clearly other tests will ?sporadically hit it)

@jozkee
Copy link
Member Author

jozkee commented Aug 11, 2021 •
edited
Loading

@danmoseley in theory the steps to test this is have a callback in the Timer's queue AFTER you disposed the Timer (which in this case is triggered by PhysicalFilesWatcher.Dispose() -> Timer.Dispose()) AND the file no longer exists. Not 100% sure how to translate that to code/unit test.

@ChayoteJarocho ChayoteJarocho mentioned this pull request Aug 11, 2021
Merged
@danmoseley danmoseley mentioned this pull request Aug 11, 2021
Closed
@danmoseley
Copy link
Member

danmoseley commented Aug 12, 2021 •
edited
Loading

Lots and lots of staging failures. I went through most

failure in Mono Windows Microsoft.Extensions.Hosting.Tests.ConsoleLifetimeExitTests.EnsureEnvironmentExitDoesntHang already tracked by #57184

The test libraries on Catalyst reports show up as "The Helix Work Item failed. Often this is due to a test crash or infrastructure failure. See the Helix Test Logs tab in the Results page of Azure DevOps." but when I look at artefacts, there's a testResults.Xml present with results in it -- @steveisok ? I think that's more interesting to fix than the actual failures.
example failures

  • hitting the UTF7 issue "Support for UTF-7 is disabled"
  • "System.PlatformNotSupportedException : System.Drawing.Common is not supported on non-Windows platforms
  • TestJapaneseCalendarDateParsing
  • etc

Android has the same issue with not extracting results from the test results XML

these two failures both seem related to appcompat switches?

    <collection total="1" passed="0" failed="1" skipped="0" name="Test collection for System.IO.Tests.Net5CompatSwitchTests" time="0.003">
      <test name="System.IO.Tests.Net5CompatSwitchTests.LegacySwitchIsHonored" type="System.IO.Tests.Net5CompatSwitchTests" method="LegacySwitchIsHonored" time="0.0028879" result="Fail">
        <failure exception-type="Xunit.Sdk.ContainsException">
          <message><![CDATA[Assert.Contains() Failure\nNot found: Net5Compat\nIn value:  System.IO.Strategies.BufferedFileStreamStrategy]]></message>
          <stack-trace><![CDATA[   at System.IO.Tests.Net5CompatSwitchTests.LegacySwitchIsHonored() in /_/src/libraries/System.IO.FileSystem/tests/Net5CompatTests/Net5CompatSwitchTests.cs:line 23
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)]]></stack-trace>
        </failure>

    <collection total="1" passed="0" failed="1" skipped="0" name="Test collection for System.IO.Tests.DisabledFileLockingSwitchTests" time="0.027">
      <test name="System.IO.Tests.DisabledFileLockingSwitchTests.ConfigSwitchIsHonored" type="System.IO.Tests.DisabledFileLockingSwitchTests" method="ConfigSwitchIsHonored" time="0.0268309" result="Fail">
        <failure exception-type="Xunit.Sdk.EqualException">
          <message><![CDATA[Assert.Equal() Failure\nExpected: False\nActual:   True]]></message>
          <stack-trace><![CDATA[   at System.IO.Tests.DisabledFileLockingSwitchTests.ConfigSwitchIsHonored() in /_/src/libraries/System.IO.FileSystem/tests/DisabledFileLockingTests/DisabledFileLockingSwitchTests.cs:line 13
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)]]></stack-trace>

Turkish I

      <test name="System.Tests.CharTests.CasingTest(lowerForm: 'ı', upperForm: 'I', cultureName: \&quot;tr-TR\&quot;)" type="System.Tests.CharTests" method="CasingTest" time="0.0567224" result="Fail">
        <failure exception-type="Xunit.Sdk.EqualException">
          <message><![CDATA[Assert.Equal() Failure\nExpected: 'ı'\nActual:   'i']]></message>
          <stack-trace><![CDATA[   at System.Tests.CharTests.CasingTest(Char lowerForm, Char upperForm, String cultureName) in /_/src/libraries/System.Runtime/tests/System/CharTests.cs:line 1132
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)]]></stack-trace>
        </failure>

various others

No evidence that any of these are caused by this PR. We may be in a bind here where something has broken these specifically because they don't block checkin?

@steveisok
Copy link
Member

steveisok commented Aug 12, 2021 •
edited
Loading

@danmoseley #57114 should address these. There was a prior commit that stopped runtimeconfig.json from being processed. On mobile, we generate a bin file from runtimeconfig.json

@danmoseley
Copy link
Member

@steveisok thanks, following back to what caused that it seems it was #56486 but it looks like staging passed 100% before that was committed?

@danmoseley danmoseley merged commit e0f6071 into dotnet:main Aug 12, 2021
@danmoseley
Copy link
Member

The only non staging failure was the async one fixed in a parallel PR.

@jozkee jozkee deleted the nullref_physicalfileprovider branch August 12, 2021 00:38
@ghost ghost locked as resolved and limited conversation to collaborators Sep 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danmoseley danmoseley danmoseley approved these changes

@carlossanlop carlossanlop carlossanlop approved these changes

Assignees

@jozkee jozkee

Labels
area-Extensions-FileSystem
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jozkee @danmoseley @steveisok @carlossanlop