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

Remove GetTempFileName #8963

Closed
wants to merge 3 commits into from
Closed

Remove GetTempFileName #8963

wants to merge 3 commits into from

Conversation

jbagga
Copy link
Contributor

@jbagga jbagga commented Jun 24, 2017

Addresses aspnet/Coherence-Signed#510

cc @smitpatel @Eilon

@dnfclas
Copy link

dnfclas commented Jun 24, 2017

@jbagga,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@smitpatel
Copy link
Contributor

Does the new API create the file?

@bricelam
Copy link
Contributor

bricelam commented Jun 26, 2017

No. It also risks collisions.

@bricelam
Copy link
Contributor

My question is: Why aren't people deleting their temporary files? I've always wrapped these in a try..finally block.

@smitpatel
Copy link
Contributor

Looks like false alarm then.

@jbagga
Copy link
Contributor Author

jbagga commented Jun 26, 2017

@Eilon and I discussed this and he strongly suggests we use the new API. Other processes may use GetTempFileName which has caused conflicts before. You are welcome to suggest other possible solutions, other APIs etc.

@ajcvickers
Copy link
Member

@Eilon @jbagga We feel the new code is making it more flaky. If we really feel this is an issue, then please work with @bricelam and @smitpatel to come up with a solution that will be at least as robust as the code that is being replaced.

@Eilon
Copy link
Member

Eilon commented Jul 5, 2017

The problem is that other components don't clean up their temporary files, so then GetTempFile will fail 100% of the time with no recovery. These "other components" could be any Windows app that uses this API, which is a zillion apps, and not all are well-written. This is a real issue that we've hit in other places, not just a theoretical defensive move.

In what way do we think the new code is flaky? The new code is based on crypto APIs, so if you find a way to cause a collision, there's probably a much bigger problem...

@ajcvickers
Copy link
Member

@bricelam @smitpatel Can one of you come up with a solution that we don't think is flakey that doesn't use GetTempFileName?

@Eilon Is there an external issue that you know of tracking the problem with GetTempFileName? It seems like we're saying that we should never use this, which means that probably nobody should ever be using it until it gets fixed.

@jbagga
Copy link
Contributor Author

jbagga commented Jul 7, 2017

Moved to 2.1.0

cc @Eilon

@ajcvickers
Copy link
Member

Merged.

@ajcvickers ajcvickers closed this Jul 7, 2017
@jbagga jbagga deleted the jbagga/GetTempFileName branch July 7, 2017 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants