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

instanceof fs/promises FileHandle #50857

Closed
janis-ab opened this issue Nov 22, 2023 · 4 comments
Closed

instanceof fs/promises FileHandle #50857

janis-ab opened this issue Nov 22, 2023 · 4 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@janis-ab
Copy link

What is the problem this feature will solve?

I am writing a function that takes FileHandle as one of it's arguments, throughout the code i must ensure that it is a valid file handle and return appropriate error otherwise. Although documentation states there is a class FileHandle, actually it is not accessible AFAIK.

Have i missed something? Or is it really not exported? If not, then why is it not exported? Is it too complex to implement that or because it would be a bad design, or any other reasoning?

const fs = require('fs/promises');

/* Someone is using a function from my module and calls a function that expects first argument to be a FileHandle, i can not control the arguments that are passed to this function, but i can control return values. */

async function myFunction(fh /* + other args */){
    /* There goes some code.

        Before calling fh.write here i would like to ensure that fh is instance of FileHandle like so:
        if(fh instanceof fs.FileHandle )
        or
        fs.isFileHandle(fh)

        and if it is not, then throw specific exception, return error, or whatever that is more appropriate. 
     */
    await fh.write(data);

    /* more nonrelevant code */
}

What is the feature you are proposing to solve the problem?

Export fs/promises FileHandle constructor. (This one gives more power to programmer and solves my task as well.)

or

Create fs.isFileHandle function that returns true/false. (This would be enough to solve my current task)

What alternatives have you considered?

1. Skip checking fh argument, assume it is FileHandle and just call the methods.

In case of bad function argument, node would just throw exception and caller would have to handle it.

The problem with this is that it is somewhat uncontrollable of what kind of exceptions the caller will get, because it will depend on the argument he passes and how wrong it is.

Instead of that i could give a nice and clean explanation by throwing custom exception or returning appropriate error code.

2. Skip checking fh argument, assume it is FileHandle, but wrap it in try-catch block every time i call async method onto it.

This makes code unreadable and i would have to handle all different exception cases if i would like to propagate any "valid" exception that was raised by FileHandle upwards to caller.

3. check if fh argument is object that has methods like write, read, etc. and only then call.

This does not protect against cases when function names match, but they work differently since wrong object was passed by accident as fh.

OTOH it gives some flexibility, since caller could implement his "own fh" that emulates real FileHandle for some purposes.

4. Create my own proxy object around FileHandle and check instanceof "That" (never accept FileHandle directly).

Too much code for simple thing...

5. Create my isFileHandle function (or forcefully expose constructor of fs.FileHandle)

While it can be achieved with code like in issue 43821 the result is very fragile, because it is not portable across environments, not all are guaranteed to have /dev/null, it must be called from async context, thus it invites to write fragile code (i.e. user forgets to await on isFileHandle or forgets to call "special module init functions").

Similar question was at issue 3523.

6. Test fh for most often expected bad values

I can interrupt execution if fh is integer, if it is string, if it is null, if undefined, etc. etc. but with this approach i can handle a limited number of scenarios.

@janis-ab janis-ab added the feature request Issues that request new features to be added to Node.js. label Nov 22, 2023
@aduh95
Copy link
Contributor

aduh95 commented Nov 22, 2023

In JS there's no way really to know whether an object is actually from a class or not, we always have to use some proxy, and there's no perfect proxy, so it's a best-effort kind-of-thing. E.g. instanceof checks can be mutated by users, and without user mutations it can return surprising results when passing objects from one realm to another.

At the end of the day, you are going to "just call the methods", so I think it makes sense to use those as proxy to validate the argument. If the object passed to the function has the necessary functions, it seems safe to assume it's either a genuine FileHandle or the user is purposefully trying to trick the system – at which point you should probably let it happen and assume the user knows what they're doing.

@janis-ab
Copy link
Author

Hi!

Thank you for a reply. I was kind of wishing this time to do a bit better than "just call the methods". Could you elaborate a little bit on "instanceof checks can be mutated by users"? Did you mean the case when user overwrites properties onto given object or something more sneaky? It kind of got me on a path on different thoughts of how "instanceof checks" can be tricked...

@aduh95
Copy link
Contributor

aduh95 commented Nov 23, 2023

I've opened #50874 to document the cases I have in mind. My point is that instanceof won't stop a sneaky user to pass non-FileHandle instances, and at the same time it might get in the way of a user passing genuine FileHandle instances but from a different realm for some whatever reason.

@janis-ab
Copy link
Author

While casual programmer most probably would not do such a sneaky tricks, the biggest issue in my opinion to this is with genuine FileHandle not being detected when passed between contexts and that can happen for valid reasons.

The documented case clearly shows why the use of instanceof should be avoided.

Thank you for your input, it is really valuable for me and hopefully for people in future who stumble upon this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

2 participants