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

Implement new FileStreamStrategy for Unix #53809

Closed
adamsitnik opened this issue Jun 7, 2021 · 7 comments · Fixed by #55191
Closed

Implement new FileStreamStrategy for Unix #53809

adamsitnik opened this issue Jun 7, 2021 · 7 comments · Fixed by #55191
Assignees
Milestone

Comments

@adamsitnik
Copy link
Member

#53669 (comment)

cc @stephentoub

@adamsitnik adamsitnik added this to the 6.0.0 milestone Jun 7, 2021
@adamsitnik adamsitnik added this to To do in System.IO - FileStream via automation Jun 7, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 7, 2021
@ghost
Copy link

ghost commented Jun 7, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

#53669 (comment)

cc @stephentoub

Author: adamsitnik
Assignees: -
Labels:

area-System.IO

Milestone: 6.0.0

@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label Jun 7, 2021
@teo-tsirpanis
Copy link
Contributor

I can do it, I will start working on it in after July 2.

@stephentoub
Copy link
Member

I can do it, I will start working on it in after July 2.

@teo-tsirpanis, I hadn't seen this comment, and I was inspired to do it based on your RandomAccess PR. Have you already started? I'm fairly close to done, but if you're already working on it, I can drop mine and let you run with it.

@teo-tsirpanis
Copy link
Contributor

No @stephentoub I haven't started it yet, that was going to be after the RandomAccess PR. Better not drop it though, I don't know how easy or hard it will be for me to do.

@teo-tsirpanis
Copy link
Contributor

I am currently working on it, its progress can be found on https://github.com/teo-tsirpanis/dotnet-runtime/tree/unix-file-stream-strategy, I will submit a PR once ready.

@stephentoub
Copy link
Member

stephentoub commented Jul 4, 2021

Based on your previous response ("Better not drop it though"), I have a PR ready to go; I just need to measure it with your other PR. You're of course welcome to keep working on it, though... maybe you'll come up with a better solution. :-)

@stephentoub stephentoub self-assigned this Jul 4, 2021
@teo-tsirpanis
Copy link
Contributor

teo-tsirpanis commented Jul 4, 2021

OK, better you do it, both PRs are following the same idea either way but yours is already complete and you can locally test and benchmark it (spoiler alert, I could not test the other PR on my machine, it's not powerful enough and does not even have Linux 😳). And you did some things differently better, like the OperatingSystem check for file locking on Macs instead of the pre-existing separate files I moved to FileStreamHelpers.

I reviewed your code and did not find anything I would change.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 6, 2021
System.IO - FileStream automation moved this from To do to Done Jul 8, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging a pull request may close this issue.

3 participants