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

refactoring of the fs module. #279

Open
drsm opened this issue Jan 17, 2020 · 32 comments
Open

refactoring of the fs module. #279

drsm opened this issue Jan 17, 2020 · 32 comments

Comments

@drsm
Copy link
Contributor

drsm commented Jan 17, 2020

  1. Handle arguments containing path strings in one place:
    1e9f7f5
@drsm
Copy link
Contributor Author

drsm commented Jan 20, 2020

  1. Fixed callback invocations in "fs" module. Fixes fs.readFile should be async #200.
    https://gist.github.com/drsm/3f4567cacbc0f088862b04a0b5d5d7f2 (Ready)

@drsm
Copy link
Contributor Author

drsm commented Jan 21, 2020

TODO: (in no particular order)

  • TypedArrays & ArrayBuffer
  • better testsuite
  • benchmark
  • add more encodings

@xeioex
Copy link
Contributor

xeioex commented Jan 22, 2020

Fixed callback invocations in "fs" module. Fixes #200.
https://gist.github.com/drsm/3f4567cacbc0f088862b04a0b5d5d7f2 (Ready)

Thanks, committed with some improvements: cd10a4b.

@drsm
Copy link
Contributor Author

drsm commented Jan 23, 2020

  1. Introduced fs.promises API. / Introduced fs.access and friends.
    https://gist.github.com/drsm/d950402101765c7395c36cb4051576ba (Ready)

@xeioex please take a look.
isn't it broken by design?

@xeioex
Copy link
Contributor

xeioex commented Jan 23, 2020

@drsm

isn't it broken by design?

Looks promising. Have no issues with the design.

@drsm
Copy link
Contributor Author

drsm commented Jan 25, 2020

I've updated the patch above.
The solution is compatible with nodejs except the cases like fs.promises.readFile() (our version will throw).

@xeioex
Copy link
Contributor

xeioex commented Jan 28, 2020

@drsm I am on large (Array objects) refactoring now. I plan to merge the patch this or next week.

@drsm
Copy link
Contributor Author

drsm commented Jan 30, 2020

  1. Added fs.symlink(), fs.unlink(), fs.mkdir(), fs.rmdir() and friends.
    https://gist.github.com/drsm/d0b44c1d6252a9c2a0627c15aed97c31 (outdated)

@drsm
Copy link
Contributor Author

drsm commented Jan 31, 2020

JFF

var fs = require('fs'), fsp = fs.promises;
var current;

Promise.resolve('100k fs.access(/dev/null)')
.then((name) => {
    current = name;
    console.log('start', current);
    console.time(current);
})
.then(() => {
    console.time(current + ' sync');
    for (var i = 0; i < 100000; ++i) {
        fs.accessSync('/dev/null');
    }
    console.timeEnd(current + ' sync');
})
.then(() => {
    console.time(current + ' promise');
    var n = 100000;
    var f = () => {
        if (n--) {
            return fsp.access('/dev/null').then(() => f());
        }
        console.timeEnd(current + ' promise');
        return Promise.resolve();
    };

    return f();
})
/*.then(() => {
    var f = async () => {
        console.time(current + ' async');
        for (var i = 0; i < 100000; ++i) {
            await fsp.access('/dev/null');
        }
        console.timeEnd(current + ' async');
    };

    return f();
})*/
.then(() => {
    return new Promise((resolve, reject) => {
        console.time(current + ' callback');
        var n = 100000;
        var f = (err) => {
            if (err) {
                reject(err);
                return;
            }
            if (n--) {
                fs.access('/dev/null', f);
                return;
            }
            console.timeEnd(current + ' callback');
            resolve();
        };

        f();
    })
})
.then(() => {
    console.timeEnd(current);
    console.log('stop', current);
    current = void 0;
})
.catch((e) => {
    console.log(current, 'failed', e)
});
$ node fs_bench.js 
start 100k fs.access(/dev/null)
100k fs.access(/dev/null) sync: 131.249ms
100k fs.access(/dev/null) promise: 1226.996ms
100k fs.access(/dev/null) callback: 965.285ms
100k fs.access(/dev/null): 2326.356ms
stop 100k fs.access(/dev/null)
$ build/njs fs_bench.js 
start 100k fs.access(/dev/null)
100k fs.access(/dev/null) sync: 123.637338ms
100k fs.access(/dev/null) promise: 426.900264ms
100k fs.access(/dev/null) callback: 151.360074ms
100k fs.access(/dev/null): 781.825911ms
stop 100k fs.access(/dev/null)

@xeioex
Copy link
Contributor

xeioex commented Feb 7, 2020

@drsm

Introduced fs.promises API. / Introduced fs.access and friends.
https://gist.github.com/drsm/d950402101765c7395c36cb4051576ba (Ready)

Thanks, committed (096f5aa, d71a881) with some refactoring included.

@drsm
Copy link
Contributor Author

drsm commented Feb 10, 2020

  1. Added fs.symlink(), fs.unlink(), fs.realpath() and friends.
    https://gist.github.com/drsm/3d0402f41dc332b3c87bbe5dfcb7892e

@xeioex
Copy link
Contributor

xeioex commented Feb 18, 2020

@drsm

Added fs.symlink(), fs.unlink(), fs.realpath() and friends.
https://gist.github.com/drsm/3d0402f41dc332b3c87bbe5dfcb7892e

Thanks, committed with the following patch over .

@drsm
Copy link
Contributor Author

drsm commented May 11, 2020

@xeioex

please take a look:
https://gist.github.com/drsm/23a3167637465b48ddb0b82fc5481516

some code was taken from here

@drsm
Copy link
Contributor Author

drsm commented May 12, 2020

Added fs.mkdir(), fs.rmdir() and friends.

https://gist.github.com/drsm/4785ae96a8b1ccee27a56c2919d6ba18

@xeioex
Copy link
Contributor

xeioex commented May 12, 2020

@drsm

Thanks Artem. Will take a look at the end of the week.

@xeioex
Copy link
Contributor

xeioex commented May 18, 2020

@drsm

please take a look:
https://gist.github.com/drsm/23a3167637465b48ddb0b82fc5481516

It seems to me that the concept is very straightforward to put any copyrights here.

@xeioex
Copy link
Contributor

xeioex commented May 18, 2020

@drsm

Added fs.mkdir(), fs.rmdir() and friends.

BTW, why not support recursive mode also?

@xeioex xeioex added the WIP label May 18, 2020
@xeioex
Copy link
Contributor

xeioex commented May 26, 2020

@drsm

Added fs.mkdir(), fs.rmdir() and friends.
https://gist.github.com/drsm/4785ae96a8b1ccee27a56c2919d6ba18

Committed, thanks.

Also added missing parts for rename() method in 0f28a80.

@drsm
Copy link
Contributor Author

drsm commented May 27, 2020

@xeioex

BTW, why not support recursive mode also?

I'll to do it.

@drsm
Copy link
Contributor Author

drsm commented Jul 15, 2020

@xeioex

Please take a look:

Improved fs.mkdir() to support recursive directory creation.
https://gist.github.com/drsm/342c4300b17b90cf749f3e316fc3b403

@xeioex
Copy link
Contributor

xeioex commented Jul 24, 2020

@drsm

Please take a look:

Improved fs.mkdir() to support recursive directory creation.
https://gist.github.com/drsm/342c4300b17b90cf749f3e316fc3b403

Thanks, committed with some changes. Most notably avoiding any changes to const char * memory.

@drsm
Copy link
Contributor Author

drsm commented Jul 25, 2020

@xeioex

Improved fs.rmdir() to support recursive directory removal.
https://gist.github.com/drsm/38714fecb523293d70296741cb62eb02

The upstream solution is still buggy

@xeioex
Copy link
Contributor

xeioex commented Jul 27, 2020

@drsm

Take a look, https://gist.github.com/xeioex/cf5d4d451642510777552614ca1396af

Coverity (static analyser) found the following issue with the previous mkdir patch.

@drsm
Copy link
Contributor Author

drsm commented Jul 27, 2020

@xeioex

Take a look, https://gist.github.com/xeioex/cf5d4d451642510777552614ca1396af

Coverity (static analyser) found the following issue with the previous mkdir patch.

Yes, there is a race condition.
I was unsure about error codes, so decided to stat it first :).
https://github.com/freebsd/freebsd/blob/master/bin/mkdir/mkdir.c#L182

The fix looks fine to me.

@drsm
Copy link
Contributor Author

drsm commented Jul 27, 2020

@xeioex

The fix looks fine to me.

BTW, we may lose an original mkdir() errno after stat:
https://github.com/openbsd/src/blob/master/bin/mkdir/mkdir.c#L150
but i don't think it's a serous issue.

@xeioex
Copy link
Contributor

xeioex commented Aug 10, 2020

@drsm

Improved fs.mkdir() to support recursive directory creation.
https://gist.github.com/drsm/342c4300b17b90cf749f3e316fc3b403

https://gist.github.com/144da2743e6c6230bcb6cddb22ea951d

I had to implement njs_file_tree_walk() from scratch because ftw.h is a total mess, even though it is a POSIX interface.
Alpine (libc musl) vs Ubuntu (glibc) vs Freebsd all they respond differently to _XOPEN_SOURCE feature macros.

@drsm
Copy link
Contributor Author

drsm commented Aug 11, 2020

@xeioex
Great Job!

@xeioex
Copy link
Contributor

xeioex commented Aug 11, 2020

@drsm

Committed in f8c8fd8, thanks.

@xeioex
Copy link
Contributor

xeioex commented Sep 28, 2020

@drsm

I added support for Buffer object in fs module, feel free to catch any issues.

@drsm
Copy link
Contributor Author

drsm commented Sep 28, 2020

@xeioex

Thanks!

This place is questionable:

>> fs.realpath(Buffer.from('/none'), (e) => void console.log(e.path))
undefined
Buffer [47,110,111,110,101]

vs.

> fs.realpath(Buffer.from('/none'), (e) => void console.log(e.path))
undefined
> /none

@xeioex
Copy link
Contributor

xeioex commented Oct 1, 2020

@drsm

This place is questionable:

Agree, take a look
https://gist.github.com/xeioex/8d3fa0a42c04042693c9db5196f3bd52

it also eliminates dynamic allocation for path arguments.

@xeioex
Copy link
Contributor

xeioex commented Aug 20, 2022

Hi @drsm,

You may help by testing the following patch

Added fs.FileHandle.                                                                                                                          
                                                                                                                                              
The following methods are implemented:                                                                                                        
    - fs.openSync(path[, flag[, mode]])                                                                                                       
    - fs.promises.open(path[, flag[, mode]])                                                                                                  
    - fs.fstatSync(fd)                                                                                                                        
    - fs.readSync(fd, buffer, offset[, length[, position]])                                                                                   
    - fs.writeSync(fd, buffer, offset[, length[, position]])                                                                                  
    - fs.writeSync(fd, string[, position[, encoding]])                                                                                        
                                                                                                                                              
The following properties of FileHandle are implemented:                                                                                       
    - filehandle.fd                                                                                                                           
    - filehandle.read(buffer, offset, [length[, position]])                                                                                   
    - filehandle.stat()                                                                                                                       
    - filehandle.write(buffer, offset, [length[, position]])                                                                                  
    - filehandle.close()  

to test njs

test/test262  test/fs/methods.t.js
TOTAL: PASSED [1/1]

to compare with node

test/test262  --binary=node test/fs/methods.t.js
TOTAL: PASSED [1/1]

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

2 participants