This repository has been archived by the owner. It is now read-only.

fs: deprecate exists() and existsSync() #8418

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@cjihrig

cjihrig commented Sep 21, 2014

fs.exists() does not currently support Node's standard callback signature. This commit adds errback support while maintaining backwards compatibility. Closes #8369.

lib/fs.js Outdated
// If the callback is passed more than one argument, then call back
// with error and exists. Otherwise, use the legacy behavior, and only
// call back with exists.
if (callback.length > 1)

This comment has been minimized.

@yorkie

yorkie Sep 21, 2014

Member

-1 because callback.length may have a big performance problem :(
http://jsperf.com/function-length-performance/8

This comment has been minimized.

@yorkie

yorkie Sep 21, 2014

Member

suggest access to node version like useErrback = version > '0.10.32' cached at initialize stage, and here we just get: if (useErrback)

This comment has been minimized.

@cjihrig

cjihrig Sep 21, 2014

That performance hit is ugly. There is no point in checking the node version though. I guess there are a few options.

  • Do nothing and close this PR. This function isn't recommended for use anyway.
  • Take the performance hit, maintain backward compatibility, and support both styles (current implementation).
  • Introduce a breaking change and support only the errback style.

This comment has been minimized.

@rlidwka

rlidwka Sep 21, 2014

or officially deprecate this function, and remove it from documentation (maybe add fs.access instead)

This comment has been minimized.

@vkurchatkin
@trevnorris

This comment has been minimized.

trevnorris commented Sep 22, 2014

@rlidwka

or officially deprecate this function, and remove it from documentation (maybe add fs.access instead)

Good potential idea, and that is a safe way to address the deprecation cycle Node uses. Though whether we actually add similar behavior back in is debatable. The only slight potential use is to try to avoid throwing when opening a file synchronously. But that is essentially a race condition, and I don't believe Node should natively support a workflow that allows that type of behavior.

@cjihrig Mind updating the PR and simply deprecating fs.exists{Sync}()?

@cjihrig cjihrig force-pushed the cjihrig:8369 branch from aeb381c to 376cfbd Sep 24, 2014

@cjihrig cjihrig changed the title from fs: add errback style support to exists() to fs: deprecate exists() and existsSync() Sep 24, 2014

@cjihrig

This comment has been minimized.

cjihrig commented Sep 24, 2014

@trevnorris I've updated the PR to deprecate those methods. I also had to update test/common.js, as the deprecation message was causing tests to fail when no output was expected.

@@ -641,6 +641,8 @@ callback, and have some fallback logic if it is null.
## fs.exists(path, callback)
This function is **deprecated**. Use [fs.open()][] instead.

This comment has been minimized.

@trevnorris

trevnorris Sep 24, 2014

Get rid of the. "Use ... instead". The user should be able to figure out what they need to be doing. :)

@@ -658,6 +660,8 @@ and handle the error when it's not there.
## fs.existsSync(path)
This function is **deprecated**. Use [fs.openSync()][] instead.

This comment has been minimized.

@trevnorris
@trevnorris

This comment has been minimized.

trevnorris commented Sep 24, 2014

Nice. One little nit, other than that LGTM.

@tjfontaine Thoughts on properly deprecating fs.exists{Sync}()? Docs already say they shouldn't be used.

@cjihrig cjihrig force-pushed the cjihrig:8369 branch from 376cfbd to edb6c3c Sep 25, 2014

@cjihrig

This comment has been minimized.

cjihrig commented Sep 25, 2014

@trevnorris updated

@othiym23

This comment has been minimized.

othiym23 commented Sep 25, 2014

I'm going to kibbitz just a tiny bit and say to @trevnorris that I don't think the world will explode if there's a hint or two in the documentation towards what people might want to use instead of the deprecated functions. A lot of people really don't know what they're supposed to do instead of use exists().

@saghul

This comment has been minimized.

Member

saghul commented Sep 25, 2014

@othiym23 fs functions tend to align with POSIX, maybe adding fs.access (which uses acess(2)) it's not a bad idea then?

@trevnorris

This comment has been minimized.

trevnorris commented Sep 25, 2014

@saghul Sounds good. We'll need to use the F_OK flag (though I'm sure you already knew that :-).

@cjihrig This is a big ask, but mind adding fs.access{Sync}() (@saghul can you give details on how to implement this?) then add this as an alternative in the documentation?

@cjihrig

This comment has been minimized.

cjihrig commented Sep 25, 2014

I can certainly try. I will likely need some guidance from @saghul if there is a libuv component that needs to be implemented.

@saghul

This comment has been minimized.

Member

saghul commented Sep 25, 2014

@cjihrig yeah, we'll need some uv_fs_access function. It would look something like this:

int uv_fs_access(uv_loop_t* loop, uv_fs_t* req, const char* path, int flags, uv_fs_cb cb)

On Unix this would be a thin wrapper on top of access(2). On Windows, you need to use GetAttributesW.

Feel free to ping me if you need help.

@cjihrig

This comment has been minimized.

cjihrig commented Sep 30, 2014

@saghul would you mind taking a quick look at this commit? It's just the Unix side of things, but wanted to make sure I was on the right track. Thanks.

@saghul

This comment has been minimized.

Member

saghul commented Sep 30, 2014

@cjihrig looks solid, keep up the good work :-) Also extra karma for adding docs without me complaining about it! 👍

@trevnorris

This comment has been minimized.

trevnorris commented Sep 30, 2014

Left a comment about implementation details. @saghul you'll probably want to chime in on the correct approach.

@saghul

This comment has been minimized.

Member

saghul commented Oct 1, 2014

Left a comment there myself. @cjihrig please open a PR on libuv, you are halfway there already :-) About the use of _access vs GetFileAttributesW, I believe we tend to prefer the NT functions over the CRT ones (except when too involved). @piscisaureus can you confirm?

Either way, in this case using GetFileAttributesW should be pretty straightforward:

#ifndef F_OK
#define F_OK 0
#endif
#ifndef R_OK
#define R_OK 4
#endif
#ifndef W_OK
#define W_OK 2
#endif
#ifndef X_OK
#define X_OK 1
#endif

...

attr = GetFileAttributesW(req->pathw);
req->status = (attr != INVALID_FILE_ATTRIBUTES) &&
                      (!(mode & W_OK) || !(attr & FILE_ATTRIBUTE_READONLY) || (attr & FILE_ATTRIBUTE_DIRECTORY));
...

(adapted from CPython's source)

Those defines should probably be put on uv-win.h so Windows users get them when including uv.h.

@piscisaureus

This comment has been minimized.

Member

piscisaureus commented Oct 1, 2014

@saghul @cjihrig

Indeed, we strongly prefer WIN32 or NTAPI functions over crt functions.

As for the GetFileAttributesW suggestion - you should be aware that this is not a very effictive test whether the user has access to the file, because windows uses ACLs and not unix-style user/group/world permission.

The other thing to be aware of is that a GetFileAttributesW under the opens the file, stat()s it, then closes it.

A more reliable and faster strategy may be to just open the file with the desired permissions, and then close it again if it succeeds.

The only issue there may become that a file could be locked by another process, in which case GetFileAttributesW might succeed (it only needs the FILE_READ_ATTRIBUTES permission which would typically be available), while CreateFile with one of the more extensive permission sets (FILE_GENERIC_READ, FILE_GENERIC_WRITE, GENERIC_EXECUTE) might not.

I am not sure whether reporting "no access" when a file is locked is desirable or not.

@saghul

This comment has been minimized.

Member

saghul commented Oct 1, 2014

As for the GetFileAttributesW suggestion - you should be aware that this is not a very effictive test whether the user has access to the file, because windows uses ACLs and not unix-style user/group/world permission.

I guess it's ok. uv_fs_access will just tell us if we can access the file for reading or writing (on Windows), if there is an ACL in place I guess we'll get an error, so that's ok, we cannot access it.

The other thing to be aware of is that a GetFileAttributesW under the opens the file, stat()s it, then closes it.

Ouch, I wasn't aware of this :-/

A more reliable and faster strategy may be to just open the file with the desired permissions, and then close it again if it succeeds.

Would this be bad if the user already has the file open?

The only issue there may become that a file could be locked by another process, in which case GetFileAttributesW might succeed (it only needs the FILE_READ_ATTRIBUTES permission which would typically be available), while CreateFile with one of the more extensive permission sets (FILE_GENERIC_READ, FILE_GENERIC_WRITE, GENERIC_EXECUTE) might not.

I am not sure whether reporting "no access" when a file is locked is desirable or not.

I don't think so. In the end, this function needs to come with a big warning, because using this LBYL approach is race prone, whereas EAFP is not.

@trevnorris

This comment has been minimized.

trevnorris commented Oct 1, 2014

@saghul Ah GetFileAttributesW. I was looking for GetAttributesW (you mentioned earlier) and since I know nothing about Windows API, couldn't find it. :-)

@cjihrig

This comment has been minimized.

cjihrig commented Nov 3, 2014

@trevnorris do you still want this for 0.12? If so, any timeline on #8566 landing?

@tjfontaine

This comment has been minimized.

tjfontaine commented Nov 3, 2014

I'm not sure if I would do a full on deprecation of exists this round, but a considerable amount of documentation is desirable for how to properly cope with the deprecation of the API is required at the very least.

@trevnorris

This comment has been minimized.

trevnorris commented Nov 6, 2014

@cjihrig libuv has been updated to 1.0.0rc-2.

@cjihrig

This comment has been minimized.

cjihrig commented Nov 12, 2014

@tjfontaine do you want to deprecate in 0.13 (or whatever comes after 0.12)?

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 13, 2015

@cjihrig @trevnorris .... any reason to keep this open?

@cjihrig

This comment has been minimized.

cjihrig commented Aug 13, 2015

No. People have asked for this to be deprecated. We deprecated it in io.js, and people asked for it to be un-deprecated.

@cjihrig cjihrig closed this Aug 13, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.