raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
739 stars 93 forks source link

Function like `tmp.open` to create write stream. #46

Closed dantman closed 8 years ago

dantman commented 9 years ago

tmp.file has some nice advantages (implicit mode and exclusive) but code using the file descriptor from fs.open is rare nowadays.

An alternate version that would return a stream made with fs.createWriteStream would be nice.

silkentrance commented 9 years ago

@dantman You can create that yourself by passing in the fd option with the value returned by tmp.file a/o tmp.fileSync

Excerpt from the documentation on fs.createWriteStream

{ flags: 'w',
  encoding: null,
  fd: null,
  mode: 0666 }
raszi commented 9 years ago

@dantman How do you like @silkentrance idea, would that be feasible for you?

silkentrance commented 9 years ago

@dantman can this issue be closed?

dantman commented 9 years ago
> tmp.fileSync({})
{ name: '/var/folders/t4/lr96d_b55fd7f5cb1c_60rl80000gn/T/tmp-13038AlxsfPmS8Nif.tmp',
  fd: 21,
  removeCallback: [Function: _cleanupCallback] }
> fs.createWriteStream(_.name, {fd: _.fd})
{ _writableState: 
   { objectMode: false,
     highWaterMark: 16384,
     needDrain: false,
     ending: false,
     ended: false,
     finished: false,
     decodeStrings: true,
     defaultEncoding: 'utf8',
     length: 0,
     writing: false,
     corked: 0,
     sync: true,
     bufferProcessing: false,
     onwrite: [Function],
     writecb: null,
     writelen: 0,
     bufferedRequest: null,
     lastBufferedRequest: null,
     pendingcb: 0,
     prefinished: false,
     errorEmitted: false },
  writable: true,
  domain: 
   { domain: null,
     _events: { error: [Function] },
     _maxListeners: undefined,
     members: [] },
  _events: { finish: { [Function: g] listener: [Function] } },
  _maxListeners: undefined,
  path: '/var/folders/t4/lr96d_b55fd7f5cb1c_60rl80000gn/T/tmp-13038AlxsfPmS8Nif.tmp',
  fd: 21,
  flags: 'w',
  mode: 438,
  start: undefined,
  pos: undefined,
  bytesWritten: 0 }

vs.

> fs.createWriteStream(_.name, {fd: _.fd, flags: 'wx+'})
...
  flags: 'wx+',
  mode: 438,
...

It looks like using the fd argument to createWriteStream resets the flags used in tmp.open{Sync} instead of re-using it. The whole point of this request was that it's nice to not have to be explicit about these flags in general code (otherwise tmpName(Sync) is sufficient). So no, the fd argument is not sufficient if the resulting write stream also requires the flags to be explicit.

silkentrance commented 9 years ago

@dantman will have a look at this tomorrow.

silkentrance commented 9 years ago

@dantman this is an entirely different issue, if you will. What you basically want is passing in opening flags as an option to tmp.createTmpFile or tmp.createTmpFileSync, or is it not?

dantman commented 9 years ago

Huh? No, I want to be able to create a write stream for a temporary file the same way I would create an open fd for a temporary file. With the exact same behaviour. ie: Without suddenly being forced to declare _c.O_CREAT | _c.O_EXCL | _c.O_RDWR and the default mode myself on a nodejs fs call.

silkentrance commented 9 years ago

@dantman a temporary file is by nature a new file, thus the "w" and not the "wx+" flags. That being said, the behaviour of tmp in this case is absolutely correct. And the flags you presented are the standard flags when opening the file, see

https://github.com/raszi/node-tmp/blob/master/lib/tmp.js#L37

However, if node fs.createFileStream from an existing file descriptor diverges from the flags being used when creating the file descriptor, this is rather an issue of node itself, don´t you think?

Either way, I will have a look into this and provide you with an answer tomorrow, as I need to look up the file stream api.

silkentrance commented 9 years ago

@dantman looking at the official documentation over at node, it seems as if you are doing something wrong:

options may also include a start option to allow writing data at some position past the beginning of the file. Modifying a file rather than replacing it may require a flags mode of r+ rather than the default mode w.

I.e. just do not pass in any mode parameters and everything is fine.

I will have a look at this tomorrow evening, though.

silkentrance commented 9 years ago

@dantman I see where you are getting at, would the following API extension suffice?

function tmp.createReadStream(tmpObject) {
  return fs.createReadStream(tmpObject.name, {fd : tmpObject.fd, flags : "r"});
}
function tmp.createWriteStream(tmpObject) {
  return fs.createWriteStream(tmpObject.name, {fd : tmpObject.fd, flags : "a+"});
}

I think that registering event listeners should be left to the client, e.g. error event listener that will automatically cleanup the tmp file and so on...

Problems here might be:

'wx+' - Like 'w+' but fails if path exists.
'ax+' - Like 'a+' but fails if path exists.

Calling the above API multiple times will likely cause an error with the default flags... Would it not suffice that tmp.fileSync() used these flags and possibly failed if the path did already exist, and use createWriteStream as it is, with only the "w+"?

In addition w+ will still truncate the tmp file, do you want this to happen? I think that a+ is the better choice here, is it not?

silkentrance commented 9 years ago

@dantman @raszi

I did come up with the following API extension

/**
 * Creates a read stream from the specified tmp object.
 *
 * @param {Object} tmp object consisting of name, fd and an optional remove callback
 * @param {Function} cb optional callback
 * @returns {ReadStream|undefined} does not return when called with callback
 * @api public
 */
function _createReadStream(tmpObject, cb) {...}

/**
 * Creates a write stream from the specified tmp object.
 *
 * @param {Object} tmpObject consisting of name, fd and optional remove callback
 * @param {Function} cb optional callback
 * @returns {WriteStream|undefined} does not return when called with callback
 * @api public
 */
function _createWriteStream(tmpObject, cb) {...}

What do you think of it?

createWriteStream uses the flags O_CREAT | O_APPEND so that the API can be used multiple times and the temporary file will not be truncated.

silkentrance commented 9 years ago

@dantman Please see the proposed PR, does it meet your requirements? Please comment.

dantman commented 8 years ago

Sure, I think that covers what I was asking for.

I just don't even remember which project using tmp I needed this for.

silkentrance commented 8 years ago

@dantman can this be closed then?

dantman commented 8 years ago

Sure.

OnkelTem commented 4 years ago

So it was rolled back? I cannot find anything like this now in the exports.

Could you please provide an example how to properly createWriteStream from a temporary file?

silkentrance commented 4 years ago

@OnkelTem It was never merged as the OP did not remember the actual use case, so we just dumped it.

And in the long run we want to get rid of the file descriptor, so you will only have a name (aka resolved path) by which to work with. As such, you are free to use that, presumably unique name, as you wish. Of course, this means that you will have to open a read stream or write stream as needed, but that should be part of your application and not something that tmp is responsible for.

OnkelTem commented 4 years ago

Ah, I see. Actually I did it like this:

const tmp = require('tmp');
//...
const tmpFile = tmp.tmpNameSync({postfix: '.yaml'});
const writeStream = fs.createWriteStream(tmpFile);
readStream.pipe(writeStream);
// And later
fs.unlinkSync(tmpFile);

Is it ok?

silkentrance commented 4 years ago

@OnkelTem yes, that would be correct, especially unlinking the file by yourself as tmp will not be managing that when using tmpName*.