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

if /tmp/ not present example fails (at least on Windows) #88

Closed
cthedot opened this issue Aug 7, 2011 · 16 comments
Closed

if /tmp/ not present example fails (at least on Windows) #88

cthedot opened this issue Aug 7, 2011 · 16 comments

Comments

@cthedot
Copy link

cthedot commented Aug 7, 2011

trying the formidable example script on Windows (7, 64bit, Nodejs 0.5.3 native .exe) yields the following error:

events.js:48
throw arguments[1]; // Unhandled 'error' event
^
Error: ENOENT, The system cannot find the file specified. '\tmp\2c4faab430e45c01747400ba
48f1465f'

When adding a \tmp\ folder on hard disk volume it works flawlessly (default temp folder should be environ var TEMP or TMP which would be c:\temp or c:\Windows\temp for untampered installations at least).
I guess (maybe naively) this could be fixed easily? Giving an informative error message would be at least something. :)

BTW, nice lib!

@felixge
Copy link
Collaborator

felixge commented Aug 8, 2011

I guess I could change the default file upload location to process.cwd(). What do you think?

@Pita
Copy link

Pita commented Aug 8, 2011

Thats how we solve it at etherpad lite

var tempDirectory = "/tmp/";

//tempDirectory changes if the operating system is windows
if(os.type().indexOf("Windows") > -1)
{
  tempDirectory = "c:\\Temp\\";
}
form.uploadDir = tempDirectory;

@cthedot
Copy link
Author

cthedot commented Aug 8, 2011

Guess I am not competent enough regarding process.cwd() but IMHO Pitas approach seems a bit questionable. At least my Win7 install has no c:\temp folder (I think default is normally c:\windows\temp but that also assumes install dir of Windows is the default).
Is there no way to check for a folder and if not present fall back to process.cwd() which seems logical (from my naive standpoint :).
Actually I just checked and why not try to use process.ENV.TEMP or process.ENV.TMP (default WIndows install sets TEMP in all cases at least I am quite sure)? Is this set on OSX/Linux? Or check for Win and use process.ENV.TEMP or process.ENV.TMP or log an error message?

Sorry for not being much of a help but I am more or less new to node.

@cthedot
Copy link
Author

cthedot commented Aug 8, 2011

Should this work?

var fs = require('fs');
fs.stat(process.env.TEMP).isDirectory()

problem is that fs.stat returns simple undefined (node 0.5.3), ?

@cthedot
Copy link
Author

cthedot commented Aug 8, 2011

Sorry (my last comment is proper bullsh*t), this should be (and works too):

node -e "var fs = require('fs');fs.stat(process.env.TEMP, function(x, y
) {console.log(y.isDirectory())})"
true

(somehow this does not work on console (yet) but seems a known bug with { and } in the console)

@Pita
Copy link

Pita commented Aug 13, 2011

@cthedot You're right, looks like the tmp folder differs between windows versions. Thx for that hint

@cthedot
Copy link
Author

cthedot commented Aug 13, 2011

nice you care about Windows at all :)

@cthedot cthedot closed this as completed Aug 13, 2011
@cthedot cthedot reopened this Aug 13, 2011
@cthedot
Copy link
Author

cthedot commented Aug 13, 2011

sorry, wrong button, did not meant to close the issue

@felixge
Copy link
Collaborator

felixge commented Aug 15, 2011

Ok, I think using fs.statSync() we should choose between: '/tmp', process.env.TEMP, process.cwd() when the module is initially required. I won't have time to do the patch right away, but I'd merge a pull request.

@cthedot
Copy link
Author

cthedot commented Sep 11, 2011

I tried to implement this here https://github.com/cthedot/node-formidable/blob/master/lib/incoming_form.js
but as this is my very first Nodejs experience should be tested before pulled (test only on Win7-64). Any comments very welcome :)

@tommedema
Copy link

Wait.. why are you using synchronous blocking functions? My server will be dealing with tons of form requests. Having each form request block would be like hell on earth. :)

@cthedot
Copy link
Author

cthedot commented Sep 11, 2011

actually a good question, in Felix last comment he suggested it, async is no problem here?

@felixge
Copy link
Collaborator

felixge commented Sep 11, 2011

I will take care of this, I mean to use sync functions when the module is loading.

@tommedema
Copy link

Hmm right, I still don't see the necessity though. It feels wrong to use blocking functions when there is no need.

I assume it's not really an issue though when it's solely during initialization of the module.

@felixge
Copy link
Collaborator

felixge commented Sep 11, 2011

I assume it's not really an issue though when it's solely during initialization of the module.

require is blocking, and actually performs much more I/O than the stat calls will. It is not an issue.

@felixge
Copy link
Collaborator

felixge commented Sep 15, 2011

Released as v1.0.4.

ChALkeR pushed a commit to ChALkeR/node-formidable that referenced this issue May 13, 2016
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

No branches or pull requests

4 participants