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

tempname creates file on Windows but not on *NIX #9053

Closed
ghost opened this issue Nov 18, 2014 · 29 comments
Closed

tempname creates file on Windows but not on *NIX #9053

ghost opened this issue Nov 18, 2014 · 29 comments
Labels
breaking This change will break code help wanted Indicates that a maintainer wants help on an issue or pull request system:windows Affects only Windows
Milestone

Comments

@ghost
Copy link

ghost commented Nov 18, 2014

As noted by @tkelman. tempname on Windows does not mean the same thing that it means on *NIX. For *NIX, the relevant docs state that; "The tmpnam() and tempnam() functions return a pointer to a file name on success, and a null pointer on error", while on Windows; "Creates a name for a temporary file. If a unique file name is generated, an empty file is created and the handle to it is released; otherwise, only a file name is generated". Should we accept this discrepancy as a part of our API?

@tkelman
Copy link
Contributor

tkelman commented Nov 18, 2014

Pretty sure the discrepancy is in tempname, not mktemp, since on unix the Julia mktemp function ccalls posix mkstemp

The mkstemp() function makes the same replacement to the template and creates the template file, mode 0600, returning a file descriptor opened for reading and writing. This avoids the race between testing for a file's existence and opening it for use.

@ghost ghost changed the title mktemp creates file on Windows but not on *NIX tempname creates file on Windows but not on *NIX Nov 18, 2014
@ghost
Copy link
Author

ghost commented Nov 18, 2014

@tkelman: Thanks, spotted it as I was changing it. =)

@ghost
Copy link
Author

ghost commented Nov 18, 2014

I would probably vote for redefining tempname as:

function tempname()
    (f, io) = mktemp()
    close(io)
    f
end

The only problem being that it renders tempname rather useless in comparison to mktemp.

It should also be noted that the documentation currently is at least misleading for Windows.

@tkelman
Copy link
Contributor

tkelman commented Nov 18, 2014

I like how http://man7.org/linux/man-pages/man3/tempnam.3.html says

Never use this function. Use mkstemp(3) or tmpfile(3) instead.

That might be a vote for deprecating tempname. It looks like the only thing we might lose in that case would be the Windows-only uunique input, I'll have to look at MSDN again to figure out what that does.

@nalimilan
Copy link
Member

+1 for deprecation unless there's a good use case for not using mktemp.

If tempfile gets deprecated and Windows does not provide a good API for it, a stop-gap can be to simply generate a random number and use that for the file name.

@tkelman
Copy link
Contributor

tkelman commented Nov 18, 2014

Opinions, @johnmyleswhite and/or @vtjnash? According to blame it looks like you did most of the writing and rewriting of these functions.

@ghost
Copy link
Author

ghost commented Nov 19, 2014

I think I will side with the deprecation camp. Even if we agree on tempname creating a file and returning only the filename, the function name would be very misleading.

@vtjnash
Copy link
Member

vtjnash commented Nov 19, 2014

the windows API (like the linux API) is entirely in userspace. it's fine to emulate the whole thing. the windows api creates (and closes) the file to help with the race condition issue mentioned above (and because it uses sequential search for a filename, rather than whatever it is that <insert name of your favorite *nix> does)

while tempnam is not recommended, there are applications that require you to pass a filename, rather than a file handle, in which case it is essential.

@ghost
Copy link
Author

ghost commented Nov 19, 2014

while tempnam is not recommended, there are applications that require you to pass a filename, rather than a file handle, in which case it is essential.

@vtjnash: I can certainly see those use-cases. If so, would you consider this solution for *NIX to be satisfactory? Also, would you agree that tempname is a rather poor name for a function that in fact creates a file?

@nalimilan
Copy link
Member

@vtjnash Yet Python has deprecated mktemp and has a very bold warning in the docs:
https://docs.python.org/3.4/library/tempfile.html#tempfile.mktemp

If the function is really needed we should add the same warning in the docs.

@ninjin The name of the function comes from the Unix system call, I agree it's not great. mktemp like Python is not great either since the mk evokes creating the file to me.

@vtjnash
Copy link
Member

vtjnash commented Nov 19, 2014

@nalimilan we don't differentiate between mktemp and mkstemp, so we don't suffer from the same issue as python's mktemp

@nalimilan
Copy link
Member

@vtjnash What do you mean? tempname cannot be made completely secure because there's a race condition between the time you check the file does not exist and the time you create it. The only secure API is to directly create the file a return a handler.

@ghost
Copy link
Author

ghost commented Nov 21, 2014

I think the following questions should summarise the discussion so far:

  1. Do we agree that it is not desirable that tempname behaves differently depending on the platform?
  2. If so, do we agree that, following the same argument as for mktemp, the current behaviour on Windows should be the default? That is, creating the file.
  3. If so, do we agree that if a function creates a file and returns the path to it, tempname is a poor name?
  4. If so, and we choose to keep the functionality, what should the new name of the function be?

I would answer as follows:

  1. Yes, it is our duty to have a portable unified API.
  2. Yes, safety should be the default.
  3. Yes, the current name would be very confusing.
  4. How about mktempname? It preserves the similarity to the previous name, while making clear that it creates something.

@nalimilan
Copy link
Member

If tempname creates the file, then what's the difference with mktemp?

@ghost
Copy link
Author

ghost commented Nov 21, 2014

If tempname creates the file, then what's the difference with mktemp?

It would not return the handle, but rather close it. The primary usage of this would be along the lines of what @vtjnash mentioned, when some third-party code out of your control requires a file path.

@nalimilan
Copy link
Member

So it would return a path to an existing file? That would be confusing, as most likely third-party code which does not accept a handle won't expect the file to exist either (which means you'll have to remove the file manually).

@tkelman
Copy link
Contributor

tkelman commented Nov 22, 2014

Related to this issue, #8942 causes the pkg test to fail, but only on Windows, because tempname creates a file where Pkg.init() is trying to create a directory. It looks like that should have used tempdir actually, in a use case where you want a name only and not to create a file if what you're trying to create is a new temporary directory.

waTeim pushed a commit to waTeim/julia that referenced this issue Nov 23, 2014
@garborg
Copy link
Contributor

garborg commented Feb 14, 2015

This issue (and my ignorance thereof) had DataFrames.jl tests failing on Windows, FWIF.

@tkelman
Copy link
Contributor

tkelman commented Feb 14, 2015

Ah, is that what that was? Yeah at the very least we need better docs here, if not some way of making these platform differences less likely to cause issues.

In response to @ninjin's 4 items above, I'd say

  1. Yeah people are going to keep getting hit by this if we don't do anything about it.
  2. Probably yes. If something expects a temporary name as a string but requires it to not yet be created, these are probably not the right functions to be using. We should document this better.
  3. Yes.
  4. mktempname could work. I don't have any better ideas.

@nalimilan
Copy link
Member

Let's add a big warning to the docs for tempname, and make it simply generate a random string on Windows. Anyway there's no way to make this function completely safe, people should use mktemp.

@tkelman
Copy link
Contributor

tkelman commented Feb 14, 2015

Uh, we already have a randstring function. That sounds more like a vote for deprecating tempname and replacing it with some combination of joinpath, tempdir, and randstring...

@nalimilan
Copy link
Member

Yeah, not having tempname and letting people doing it by hand would make it more obvious that it's not a recommended way of generating temporary files.

@JeffBezanson
Copy link
Member

Conclusion from triage: deprecate this to joinpath(tempdir(), randstring()) and warn against it.

@JeffBezanson JeffBezanson added the help wanted Indicates that a maintainer wants help on an issue or pull request label Aug 10, 2017
@StefanKarpinski
Copy link
Member

Yikes, that seems pretty nasty to use for a very common pattern. I understand that the UNIX and Windows behaviors here are annoyingly different, but shouldn't we have a portable convenient function for getting a temporary filename without doing tempdir() and randstring()?

@tkelman
Copy link
Contributor

tkelman commented Aug 11, 2017

It's racy and not recommended even by posix to ask for a temporary filename without also creating the file, is it really a workflow we need to support with a named exported function?

@StefanKarpinski
Copy link
Member

My point is that we should have a non-racy, correct way of doing this that is standard. If that involves creating the file in the process, then that's the API we should expose. Is the decision here to tell people to replace this with joinpath(tempdir(), randstring()) but actually encourage them to use something like mktemp instead?

@JeffBezanson
Copy link
Member

Exactly, this is what mktemp is for.

@StefanKarpinski StefanKarpinski added breaking This change will break code and removed bug Indicates an unexpected problem or unintended behavior labels Jan 4, 2018
@JeffBezanson JeffBezanson added the system:windows Affects only Windows label Jan 9, 2018
JeffBezanson added a commit that referenced this issue Jan 12, 2018
also deprecate `tempname(::UInt32)` and add a warning to the docs.
JeffBezanson added a commit that referenced this issue Jan 12, 2018
also deprecate `tempname(::UInt32)` and add a warning to the docs.
JeffBezanson added a commit that referenced this issue Jan 14, 2018
also deprecate `tempname(::UInt32)` and add a warning to the docs.
JeffBezanson added a commit that referenced this issue Jan 14, 2018
also deprecate `tempname(::UInt32)` and add a warning to the docs.
JeffBezanson added a commit that referenced this issue Jan 15, 2018
also deprecate `tempname(::UInt32)` and add a warning to the docs.
JeffBezanson added a commit that referenced this issue Jan 15, 2018
also deprecate `tempname(::UInt32)` and add a warning to the docs.
JeffBezanson added a commit that referenced this issue Jan 15, 2018
also deprecate `tempname(::UInt32)` and add a warning to the docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code help wanted Indicates that a maintainer wants help on an issue or pull request system:windows Affects only Windows
Projects
None yet
Development

No branches or pull requests

7 participants