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

Recombine FileStream to simplify and avoid bugs #18556

Closed
stephentoub opened this issue Sep 14, 2016 · 3 comments
Closed

Recombine FileStream to simplify and avoid bugs #18556

stephentoub opened this issue Sep 14, 2016 · 3 comments
Assignees
Labels
area-System.IO bug help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@stephentoub
Copy link
Member

FileStream in corefx is currently implemented as a wrapper around an inner stream that actually provides the logic for the current platform, e.g. it wraps a UnixFileStream on Unix, a Win32FileStream on Windows, etc. This has several issues:

  • Adds expense to create, as we're now creating two streams where we actually only need one
  • Adds expense on every operation, as each virtual call on the FileStream then turns around and calls a virtual method on the wrapped stream
  • Leads to bugs, e.g. it looks like we currently have a bug, where in sync mode, FileStream.ReadAsync calls Win32FileStream.ReadAsync which calls base.ReadAsync which in turns queues a work item to call Read... but it's calling Read on the inner stream, not on the outer stream, which means a FileStream-derived type that overrides Read won't have its override called.

We can avoid all of these issues by recombining FileStream into a partial class like we use elsewhere in corefx. The primary trickiness comes with WinRT, where, depending on the path, we sometimes use Win32FileStream and sometimes use WinRTFileStream.

cc: @ericstj, @justinvp, @JeremyKuhne, @ianhays

@ianhays
Copy link
Contributor

ianhays commented Sep 14, 2016

+1 great idea. I'm really not a fan of the Win32FileSystem/UnixFileSystem structure either. The "Current" FileSystem variable seems like unnecessary indirection when we could just be using partial classes with conditional inclusion instead.

@ericstj
Copy link
Member

ericstj commented Sep 14, 2016

The FileSystem PAL we have today predates all of the cross-plat work and was designed primarily for the WinRT sceanario (and potentially even public exposure). Now that we have a more common pattern for x-plat and cross-compiling and we're cross-compiling anyway it makes sense to remove this abstraction.

@JeremyKuhne
Copy link
Member

I'm good with this as well.

@JeremyKuhne JeremyKuhne self-assigned this Oct 19, 2016
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO bug help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

5 participants