refactoring of the fs module.

Handle arguments containing path strings in one place:
1e9f7f5

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

TODO: (in no particular order)

TypedArrays & ArrayBuffer
better testsuite
benchmark
add more encodings

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

Thanks, committed with some improvements: cd10a4b.

@xeioex

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?

@drsm

@drsm

isn't it broken by design?

Looks promising. Have no issues with the design.

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

@drsm

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

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

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)

@drsm

@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.

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

@drsm

@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.

@xeioex

@xeioex

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

The upstream solution is still buggy

@drsm

@drsm

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

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

@xeioex

@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.

@xeioex

@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.

@drsm

@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.

@xeioex

@xeioex
Great Job!

@drsm

@drsm

Committed in f8c8fd8, thanks.

@drsm

@drsm

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

@xeioex

@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

@drsm

@drsm

This place is questionable:

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

it also eliminates dynamic allocation for path arguments.

@drsm

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]

Navigation Menu

Repository navigation

refactoring of the fs module. #279

11 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

refactoring of the fs module. #279

Description

Activity

drsm commented on Jan 20, 2020

drsm commented on Jan 21, 2020

xeioex commented on Jan 22, 2020

drsm commented on Jan 23, 2020

xeioex commented on Jan 23, 2020

drsm commented on Jan 25, 2020

xeioex commented on Jan 28, 2020

drsm commented on Jan 30, 2020

drsm commented on Jan 31, 2020

xeioex commented on Feb 7, 2020

drsm commented on Feb 10, 2020

11 remaining items

xeioex commented on Jul 24, 2020

drsm commented on Jul 25, 2020

xeioex commented on Jul 27, 2020

drsm commented on Jul 27, 2020

drsm commented on Jul 27, 2020

xeioex commented on Aug 10, 2020

drsm commented on Aug 11, 2020

xeioex commented on Aug 11, 2020

xeioex commented on Sep 28, 2020

drsm commented on Sep 28, 2020

xeioex commented on Oct 1, 2020

xeioex commented on Aug 20, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions