Disable file locking by default on iOS and tvOS - #134060
rolfbjarne wants to merge 8 commits into
Conversation
Use the existing System.IO.DisableFileLocking configuration switch with a platform-specific default for iOS and tvOS. This avoids kernel-visible advisory locks that can cause suspended apps to be terminated, while preserving explicit configuration overrides and the existing Mac Catalyst behavior.\n\nAdd platform-specific coverage that verifies file locking is disabled by default on iOS and tvOS; existing lock-dependent tests already skip based on the same runtime setting.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 30b5cc74-ca79-43d5-bbe4-65cdbc5b273e
Condition tests that require FileShare enforcement on PlatformDetection.IsFileLockingEnabled. This extends the existing System.IO.FileSystem test pattern to reflection, isolated storage, memory-mapped files, and assembly catalog tests so they are skipped when file locking defaults to disabled on iOS and tvOS.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 30b5cc74-ca79-43d5-bbe4-65cdbc5b273e
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-io |
There was a problem hiding this comment.
🟡 Changes recommended
The broad FileShare compatibility change remains unresolved, and opt-in plus Mac Catalyst coverage is missing.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Disables Unix advisory file locking by default on iOS and tvOS while preserving Mac Catalyst behavior and configuration overrides.
Changes:
- Updates the platform-specific locking default.
- Adds iOS/tvOS behavior coverage.
- Gates lock-dependent tests on
IsFileLockingEnabled.
File summaries
| File | Change |
|---|---|
src/libraries/System.Runtime/tests/System.Reflection.Tests/AssemblyNameTests.cs |
Gates locked-file reflection testing. |
src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/FileStream/ctor_str_fm_fa_fs.cs |
Tests disabled locking on iOS/tvOS. |
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs |
Changes platform locking defaults. |
src/libraries/System.IO.MemoryMappedFiles/tests/MemoryMappedFile.CreateFromFile.Tests.cs |
Gates sharing-violation testing. |
src/libraries/System.IO.IsolatedStorage/tests/System/IO/IsolatedStorage/OpenFileTests.cs |
Gates FileShare enforcement testing. |
src/libraries/System.ComponentModel.Composition/tests/System/ComponentModel/Composition/Hosting/AssemblyCatalogTests.cs |
Gates locked-file tests. |
Review details
Suppressed comments (2)
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs:41
- The new coverage only checks the effective platform default. The existing
DisabledFileLockingSwitchTestsforcesSystem.IO.DisableFileLocking=true, so it does not verify the advertised iOS/tvOS opt-in (System.IO.DisableFileLocking=falseorDOTNET_SYSTEM_IO_DISABLEFILELOCKING=0) still re-enables sharing enforcement. Please add an isolated Apple-mobile test for that override so this changed default cannot accidentally make the opt-in ineffective.
defaultValue: (OperatingSystem.IsIOS() && !OperatingSystem.IsMacCatalyst()) || OperatingSystem.IsTvOS());
src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/FileStream/ctor_str_fm_fa_fs.cs:127
- The new assertion covers iOS/tvOS but not the compatibility-critical
!OperatingSystem.IsMacCatalyst()branch.OperatingSystem.IsIOS()is also true for Mac Catalyst, so add a Mac Catalyst-specific assertion that file locking remains enabled; otherwise an accidental removal of this exclusion could pass the added tests.
[!NOTE]
This review comment was generated with GitHub Copilot.
[Fact]
[PlatformSpecific(TestPlatforms.iOS | TestPlatforms.tvOS)]
public void FileLockingDisabledByDefault()
{
Assert.False(PlatformDetection.IsFileLockingEnabled);
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Add Mac Catalyst coverage to ensure its existing file-locking default remains enabled even though OperatingSystem.IsIOS also identifies Mac Catalyst.\n\nAdd a focused iOS and tvOS test project that explicitly sets System.IO.DisableFileLocking to false and verifies both the effective runtime setting and FileShare.None enforcement.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 30b5cc74-ca79-43d5-bbe4-65cdbc5b273e
|
Tagging subscribers to 'os-ios': @vitek-karas, @kotlarmilos, @steveisok, @akoeplinger |
adamsitnik
left a comment
There was a problem hiding this comment.
@rolfbjarne the changes LGTM, but please take a look at my comment, as I think it would be nice to use src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/DisabledFileLockingTests/System.IO.FileSystem.DisabledFileLocking.Tests.csproj to keep testing file locking on iOS and tvOS.
Configure the existing disabled-file-locking test project to enable locking on iOS and tvOS, allowing its full FileStream sharing suite to continue exercising lock enforcement on those platforms.\n\nMove the platform-default assertions into the normal file-system test project so they continue validating the iOS, tvOS, and Mac Catalyst defaults without inheriting the override used by the specialized project. Remove the separate Apple-only override project added in the prior local commit.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 30b5cc74-ca79-43d5-bbe4-65cdbc5b273e
Keep ConfigSwitchIsHonored explicit about Windows always having file locking enabled while also accounting for the iOS and tvOS override used by the Unix-only specialized test project.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 30b5cc74-ca79-43d5-bbe4-65cdbc5b273e
Include iossimulator and tvossimulator in the specialized test project's Apple mobile override so the full file-locking suite enables lock enforcement in simulator CI as well as device target configurations.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 30b5cc74-ca79-43d5-bbe4-65cdbc5b273e
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (1)
src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/FileLockingDefaults.cs:14
- These assertions only inspect
SafeFileHandle.DisableFileLockingthroughPlatformDetection; they never exercise the observableFileSharebehavior. Because the existing sharing-violation tests are conditionally skipped when this flag is false, a regression where the flag reports false but a default iOS/tvOS open still takesflockwould pass. Add an iOS/tvOS test that opens aFileShare.Nonehandle and verifies a second open succeeds, while retaining the explicit-locking project for the opt-in path.
Assert.False(PlatformDetection.IsFileLockingEnabled);
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/azp run runtime-ioslike,runtime-maccatalyst,runtime-ioslikesimulator |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, thank you for providing the fix @rolfbjarne !
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/azp run runtime-extra-platforms |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
CI status analysis for the current PR head:
Note This comment was generated with GitHub Copilot. |
|
/ba-g based on the analysis above - the only iOS related failure are all known issues already existing before this change. |
|
@matouskozak Could you please review just the part about all failures being unrelate? The only change after the signoff from Steve and Adam has been a merge from main - not due to conflicts, but to bring in fix for Apple test build break to unblock parts of CI. I'm rushing this a bit since I need to start the backport to 11 and have it ready today. |
Summary
Disables advisory file locking by default on iOS and tvOS (device and simulator), while leaving Mac Catalyst — and every other platform — unchanged. The behavior remains fully configurable: setting
System.IO.DisableFileLocking=false(orDOTNET_SYSTEM_IO_DISABLEFILELOCKING=0) restores locking on those platforms.Motivation
On iOS and tvOS, RunningBoard terminates a suspended app with exception code
0xdead10ccwhen the app still holds a file lock on a file outside its data container. .NET acquires such a lock implicitly: everyFileStreamopened on Unix takes an advisoryflockto emulate WindowsFileSharesemantics, and aFileShare.Readopen maps to a sharedLOCK_SH.A common way to hit this is stack trace symbolication: when the runtime opens a PDB from inside the read-only app bundle to produce line numbers for an exception, the resulting shared lock lives for as long as the handle does. If the user backgrounds the app while that handle is open, the system kills the process. The lock provides no real value here — the app bundle is read-only and the lock is only advisory between .NET processes — but the termination is fatal and hard to diagnose.
Implementation
SafeFileHandle.DisableFileLockingon Unix already consults theSystem.IO.DisableFileLocking/DOTNET_SYSTEM_IO_DISABLEFILELOCKINGconfig switch; this change only alters thedefaultValuepassed to that lookup, so it becomestrueon iOS and tvOS:Notes on the predicate:
OperatingSystem.IsIOS()returnstruefor Mac Catalyst as well, so Mac Catalyst is explicitly excluded — it runs on macOS, is not subject to RunningBoard suspension policy, and keeps its current locking behavior.Because this goes through
AppContextConfigHelper, an explicitfalsestill wins over the new default, so anyone depending onFileShareenforcement on iOS/tvOS can opt back in.The observable consequence is that on iOS and tvOS,
FileShareis no longer enforced between processes or handles: opening a file withFileShare.Noneno longer prevents a second open, and theIOExceptionthat previously signalled a sharing violation is no longer thrown. This matches the behavior that Browser and WASI have had for some time.Test changes
Add a small iOS/tvOS-specific test asserting that file locking is in fact off by default, so a future regression in the predicate is caught directly rather than only as a cascade of sharing-violation test failures.
Most lock-dependent tests in
System.IO.FileSystemwere already conditioned onPlatformDetection.IsFileLockingEnabled, which reflects the runtime'sDisableFileLockingvalue and therefore picks up the new default automatically. The second commit extends that same pattern to the remaining tests that depend onFileSharebeing enforced:System.ReflectionGetAssemblyName_LockedFilenow usesConditionalFactonIsFileLockingEnabledSystem.IO.IsolatedStorageOpenFile_PassesFileSharenow usesConditionalFactonIsFileLockingEnabledSystem.IO.MemoryMappedFilesFileInUse_CreateFromFile_FailsWithExistingNoShareFilenow usesConditionalFactSystem.ComponentModel.CompositionConstructorN_LockedFileAsCodeBaseArgument_ShouldThrowFileLoadtests useConditionalFactIn two of these cases the new condition subsumes an existing
[SkipOnPlatform(TestPlatforms.Browser, ...)]attribute — Browser was skipped precisely because it does not honor file locking — so the attribute is removed in favor of the more general check.Validation
Built and ran locally on macOS:
./build.sh clr+libs -rc checked./build.sh clr.corelib+clr.nativecorelib+libs.pretest -rc checkedTest results:
System.IO.FileSystem.TestsSystem.IO.FileSystem.Tests(locking disabled)System.ReflectiontestsSystem.IO.IsolatedStoragetestsSystem.IO.MemoryMappedFilestestsSystem.ComponentModel.CompositiontestsThe "locking disabled" run exercises the new code path by forcing
DisableFileLockingon, which is the configuration iOS and tvOS will now get by default.Fixes #133697.
Note
This pull request description was generated with the assistance of GitHub Copilot.