[release/6.0] SafeFileHandle.Unix: don't DeleteOnClose when lock is not successful.

[tmds]

Description

In #55153 DeleteOnClose handling moved from FileStream to SafeFileHandle.

This unintentionally caused DeleteOnClose to be applied even when FileShare locking fails. As on Windows, DeleteOnClose should not take effect when sharing prevents the file from being opened.

This also swaps the order of unlink and LOCK_UN in Dispose as it was prior to #55153.
Either order should be fine.

Customer impact

Unintentional deleting of file.

Testing

Existing and new unit test.

Risk

Low. Behaves the same as previous versions of .NET.

Fixes #59995.

[ghost]

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

Issue Details

---

In #55153 DeleteOnClose handling
moved from FileStream to SafeFileHandle.

This unintentionally caused DeleteOnClose to be applied even when
FileShare locking fails. As on Windows, DeleteOnClose should not take
effect when sharing prevents the file from being opened.

This also swaps the order of unlink and LOCK_UN in Dispose as it was
prior to #55153.
Either order should be fine.

Fixes #59995.

@stephentoub @dotnet/area-system-io ptal.

This targets release/6.0 branch. It is a subset of changes from #55327 which tackles a broader issue.

Author:	tmds
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[adamsitnik]

LGTM, big thanks for the fix @tmds!

[tmds]

I made an attempt at formatting the initial comment into the 'backport' format.

@stephentoub will you look into the servicing considerations?

[adamsitnik]

@jeffhandley this PR is ready for being considered for merging to 6.0

[danmoseley]

This also swaps the order of unlink and LOCK_UN in Dispose as it was prior to #55153.
Either order should be fine.

What is the motivation for porting this part? Would it be lower risk to just port the fix itself?

[tmds]

What is the motivation for porting this part? Would it be lower risk to just port the fix itself?

The unlink and flock use distinct variables and both ignore errors. I see no additional risk by swapping them.
I don't think it is absolutely mandatory for the fix. The order is now the same as what it was for previous versions of .NET.
It's also the order we have on main since 3 days (#55327).

[danmoseley]

It's also the order we have on main since 3 days (#55327).

thanks @tmds. that in particular is a good argument.

[jeffhandley]

Approved by tactics. Some of the failing checks were in the IO area; rerunning...

[jeffhandley]

The System.Tests.TimeZoneInfoTests.TestWindowsNlsDisplayNames failure in runtime (Libraries Test Run release coreclr windows x86 Debug) is #60119, which was fixed in #60140.

[danmoseley]

WASM failures are tracked by #51961 (comment)
Windows 7 network tests that timed out - I'll open an issue.

@jeffhandley let me know if you'd like me to use my super merging powers

[danmoseley]

#60204

[danmoseley]

I'm going to be bold and merge...

[jeffhandley]

Thanks for merging, @danmoseley. And thanks for the fix, @tmds!