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

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

Closed
dantman opened this issue Feb 25, 2015 · 19 comments
Closed

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

dantman opened this issue Feb 25, 2015 · 19 comments

Comments

@dantman
Copy link

dantman commented Feb 25, 2015

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
Copy link
Collaborator

@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
Copy link
Owner

raszi commented Jun 2, 2015

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

@silkentrance
Copy link
Collaborator

@dantman can this issue be closed?

@dantman
Copy link
Author

dantman commented Aug 6, 2015

> 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
Copy link
Collaborator

@dantman will have a look at this tomorrow.

@silkentrance
Copy link
Collaborator

@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
Copy link
Author

dantman commented Aug 6, 2015

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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Collaborator

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

@dantman
Copy link
Author

dantman commented Dec 3, 2015

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
Copy link
Collaborator

@dantman can this be closed then?

@dantman
Copy link
Author

dantman commented Aug 29, 2016

Sure.

@OnkelTem
Copy link

OnkelTem commented Jun 2, 2020

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
Copy link
Collaborator

silkentrance commented Jun 3, 2020

@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
Copy link

OnkelTem commented Jun 4, 2020

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants