Njs: WIP: import module

Created on 12 Feb 2019  Â·  120Comments  Â·  Source: nginx/njs

@xeioex @drsm

Supported definite function list:

  1. introduced import/export statement. import x from y; export default {object}
  2. import native and non-native module. import fs from 'fs'; import lib from 'absolute/relative-path'
  3. limit global variable access in modules.
  4. limit return statement in module top-scope.
  5. restrict export statement in module top-scope.
  6. import statement is hoisted and export statement is sunk.
  7. non-native modules lookup procedure.
  8. limit access to filesystem in sandbox mode for non-native modules.
  9. besides..

Unsupported function list yet.

  1. besides..

Unclear function lists.

  1. check the duplicate name, import name is regarded as variable.
  2. error in open/fstat failed, now I reference nodejs. So I didn't throw error while open/fstat failed.
    I plan to improve error information as we discussed all the nodes should record line number after the feature, so this can be considered together.
  3. introduced njs_vmcode_export for disassembling.
  4. besides..

Unit tests.

  1. simple passed case.

Patch: (module-beta.patch)
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

feature
Source
picture hongzhidao

Most helpful comment

@macoomac @drsm

js_path directive is added:

http {
     js_path "/tmp/lib";
     js_path "/tmp/lib2";

     js_include http.js;
...

https://gist.github.com/xeioex/ac9ff2605fef81763b18289546d63d09

picture xeioex on 12 Mar 2019
👍3 😄1

All 120 comments

@hongzhidao

https://gist.github.com/hongzhidao/0be1692c6b10650ae69df74bf5dec3f3

+static njs_ret_t njs_module_error(njs_vm_t *vm, const char *syscall,

  • nxt_str_t *path, int errn);

BTW, why not reuse njs_fs_error()?

n = read(fd, p, sb.st_size);
+

  • if (n < 0) {
  • goto fail;
  • }
    +
  • if (n != sb.st_size) {
  • goto fail;
  • }

exception is also needed in these two places.

https://gist.github.com/hongzhidao/0be1692c6b10650ae69df74bf5dec3f3#file-njs-module-03-patch-L595
lhq.key_hash = nxt_djb_hash(name->start, name->length);;

extra ';' in two places.

  • module = nxt_mp_zalloc(vm->mem_pool, sizeof(njs_module_t));
  • if (nxt_slow_path(module == NULL)) {
  • return NULL;
  • }

njs_memory_error(vm) is needed.

xeioex picture xeioex on 12 Feb 2019
👍1

BTW, why not reuse njs_fs_error()?

I thought of it, but I found nodejs regard the native and non-native modules the same while they throw errors.

cat /tmp/test.js && node /tmp/test.js
require("./non.js");

module.js:340
    throw err;
          ^
Error: Cannot find module './non.js'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (/tmp/test.js:1:63)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)

njs_memory_error(vm) is needed.

This function confused me since there are so many places like it (return NULL), and I don't know where to call the function.

hongzhidao picture hongzhidao on 12 Feb 2019

This function confused me since there are so many places like it (return NULL), and I don't know where to call the function.

the rule of thumb, is that njs_memory_error(vm) needs to be called at least once, once memory allocation fails. most njs_*() function already do it, so it is not necessary to set it again when you call them. But, usually, when you are working with nxt_mp_alloc() failure, you need to set it.
To reduce clutter, when you have many allocations, I prefer to do gotos to a single place.

xeioex picture xeioex on 12 Feb 2019

@hongzhidao

BTW, for the future. I do not think this is a good idea to create multiple tickets for the same task. It is hard to make references from other tickets. Github conveniently hides comments from the middle for long tickets.

xeioex picture xeioex on 12 Feb 2019
🎉1

@xeioex

Take a look, please.
https://gist.github.com/hongzhidao/0d2e1a73b2664fc6f53d648a0cc9acec

  1. Introduced nxt_file_t.
    https://gist.github.com/hongzhidao/0d2e1a73b2664fc6f53d648a0cc9acec#file-njs-module-04-patch-L1247
    Now, the lexer->file record the full path, then it's resolved in scope, so each scope has its own path/dir/name, and scope->file.name is used in parser/generator/debug error information.

  2. Searching module by path.
    https://gist.github.com/hongzhidao/0d2e1a73b2664fc6f53d648a0cc9acec#file-njs-module-04-patch-L362
    Each scope has its own dir, it'll be used to calculate the absolute path with import name. See tests.

  3. Fixed return statement. In lib.js, this is a module imported outside.
    https://gist.github.com/hongzhidao/0d2e1a73b2664fc6f53d648a0cc9acec#file-njs-module-04-patch-L870

// lib.js, the following is illegal.
if (1) { return }
  1. Fixed duplicated parser error.
    https://gist.github.com/hongzhidao/0d2e1a73b2664fc6f53d648a0cc9acec#file-njs-module-04-patch-L1195
    This happens in recursive import statement.
// main.js
import x from './x.js'; // (ii) This throws njs_parser_error
// x.js
import y from './non-exist.js'; // (i) This throws njs_parser_error

https://gist.github.com/hongzhidao/0d2e1a73b2664fc6f53d648a0cc9acec#file-njs-module-04-patch-L446

  1. Improved tests.

  2. Removing njs_module_error. I'm not sure.

    I thought of it, but I found nodejs regard the native and non-native modules the same while they throw errors.

hongzhidao picture hongzhidao on 13 Feb 2019
👍2

@hongzhidao 

import c0 from 'crypto';
import c1 from 'crypto';
import c1 from 'fs';

console.log('xxx:', c1 === c0);
drsm picture drsm on 13 Feb 2019

@drsm thank you.

@xeioex it seems we need check duplicate variables.
https://github.com/nginx/njs/issues/80

hongzhidao picture hongzhidao on 13 Feb 2019 
root@node:~# cat nul.mjs 
import c from 'crypto' + 'to';

console.log(c);
root@node:~# node --experimental-modules nul.mjs 
(node:373) ExperimentalWarning: The ESM module loader is experimental.
file:///root/nul.mjs:1
import c from 'crypto' + 'to';
                       ^
SyntaxError: Unexpected token +
    at translators.set (internal/modules/esm/translators.js:43:18)

vs

$ build/njs main.js 
'c1:' {}
$ cat main.js 
import c1 from 'crypto' + 'to';

console.log('c1:', c1);
drsm picture drsm on 13 Feb 2019
👍1

@hongzhidao @xeioex

it seems we need check duplicate variables.

maybe postpone this until we have a 'const' ?

import c1 from 'crypto';
var c1 = null;
console.log('c1:', c1);
// logs null
drsm picture drsm on 13 Feb 2019

@drsm

maybe postpone this until we have a 'const' ?

Agree.

And I forgot to limit accessing global variables in modules, it's an issue.

hongzhidao picture hongzhidao on 13 Feb 2019
👍1

@drsm 

root@node:~# cat nul.mjs 
import c from 'crypto' + 'to';

console.log(c);
root@node:~# node --experimental-modules nul.mjs 
(node:373) ExperimentalWarning: The ESM module loader is experimental.
file:///root/nul.mjs:1
import c from 'crypto' + 'to';
                       ^
SyntaxError: Unexpected token +
    at translators.set (internal/modules/esm/translators.js:43:18)

vs

$ build/njs main.js 
'c1:' {}
$ cat main.js 
import c1 from 'crypto' + 'to';

console.log('c1:', c1);

Fixed, try the patch. Thank you.
https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d
Reason:
https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d#file-njs-module-05-patch-L947

hongzhidao picture hongzhidao on 13 Feb 2019

@hongzhidao

https://gist.github.com/hongzhidao/0d2e1a73b2664fc6f53d648a0cc9acec

https://gist.github.com/hongzhidao/0d2e1a73b2664fc6f53d648a0cc9acec#file-njs-module-04-patch-L1335

Once we have shorthand object literals. export default {hash: hash}; -> export default {hash};

modules search paths
nxt_file_resolve(&scope->file, &lexer->file);

I am not sure we need nxt_file_t in scopes.

I see a module lookup procedure as follows:

1) paths are provided in njs_vm_opt_t for njs_vm_create()
2) In nginx we would have js_modules directive similar to perl_modules.
3) in shell we would have -p option to add a path prefix (or NJS_PATH env var).
4) in njs_parser_module() we need njs_module_lookup(vm, name);

njs_module_lookup()  // in pseudocode
  for prefix in vm->opts.paths:
     path = prefix + name;
     fd = open(path);

     if (fd > 0) return {fd, path};

What do you think?

xeioex picture xeioex on 13 Feb 2019

njs_module_lookup() // in pseudocode
for prefix in vm->opts.paths:
path = prefix + name;
fd = open(path);

 if (fd > 0) return {fd, path};

Besides, In the case.

(1) ./build/njs /tmp/js/main.js  (note, the file is absolute path)

// /tmp/js/main.js  (2) Resolved current dir: /tmp/js
import lib1 from '../../lib1.js';  (3) Full path: /tmp/js/../../lib1.js



md5-6b40582a5c4c3d3d7475fb5e9a487970



// Repeat (2, 3) in lib.js



md5-a431af7aa9a94593adcd4c806bae5c25



njs_module_lookup()  // in pseudocode
  paths = [scope->current_dir, [vm->opts.paths]]
  for prefix in paths:
     path = prefix + name;
     fd = open(path);

     if (fd > 0) return {fd, path};
hongzhidao picture hongzhidao on 13 Feb 2019
👍1

I think the current dir should be treated first, and it works now.

Agree.

xeioex picture xeioex on 13 Feb 2019

The structure nxt_file_t solves the problem. The order is.

lexer->file = full path. (from vm_options or the result file name from import statement.
nxt_file_resolve(&scope->file, lexer->file).
scope->file.name used in error/debug information.
scope->dir used in njs_module_lookup().

What's not clear is the field 'path' in nxt_file_t. Or separate dir and name to corresponding place.
I'll think of this again.

hongzhidao picture hongzhidao on 13 Feb 2019

@hongzhidao
https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d#file-njs-module-05-patch-L1211 - set but unused

import c1 from 'crypto' + 'to';
// SyntaxError: Unexpected token "+" in 
// fixed

// but
import c1 from 'cryp' + 'to';
// SyntaxError: Cannot find module "cryp" in
drsm picture drsm on 13 Feb 2019
👍1

What's not clear is the field 'path' in nxt_file_t. Or separate dir and name to corresponding place.
I'll think of this again.

Maybe, we can store all the paths in vm->paths (nxt_array_t). And PATH_MAX seems to much to have for all the entries.

njs_module_lookup()  // in pseudocode
  paths = [scope->current_dir, [vm->opts.paths]]
  for prefix in paths:
     path = prefix + name;
     fd = open(path);

     if (fd > 0) {
         push path (nxt_str_t) to vm->paths.
         return fd;
    }
xeioex picture xeioex on 13 Feb 2019

The current_dir of scope need to be removed after quiting module lambda. It can't be reused.

hongzhidao picture hongzhidao on 13 Feb 2019

@drsm

@hongzhidao
https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d#file-njs-module-05-patch-L1211 - set but unused

import c1 from 'crypto' + 'to';
// SyntaxError: Unexpected token "+" in 
// fixed

// but
import c1 from 'cryp' + 'to';
// SyntaxError: Cannot find module "cryp" in

Updated.
https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d

We need to parse the import statement first, then parse module lambda.

hongzhidao picture hongzhidao on 13 Feb 2019
👍1

@hongzhidao

We need to parse the import statement first, then parse module lambda.

works as expected now, thanks!

drsm picture drsm on 13 Feb 2019
🎉1

@xeioex @drsm

Found a new issue.

cat /tmp/test.js && ./build/njs /tmp/test.js
function foo() {
    b;
}
ReferenceError: "b" is not defined in test.js:4  // The line number is wrong!

I'll process it. #94

hongzhidao picture hongzhidao on 13 Feb 2019

@xeioex @drsm

Patches:

  1. fixed error line number.
    https://gist.github.com/hongzhidao/7c628db04af7ac403677a2b39e1c426d
  1. updated module. (include limiting global variables access)
    https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d

Some changes:

  1. limiting global variables access.
    https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d#file-njs-module-05-patch-L1254

  1. introduced current work director in scope.
    https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d#file-njs-module-05-patch-L347

  2. changed parser lexer.
    https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d#file-njs-module-05-patch-L37

Welcome to review and test again.

hongzhidao picture hongzhidao on 14 Feb 2019

@hongzhidao

2. https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d

tests are missing.

expect tests do not pass.

before

>> JSON.parse(Error()
SyntaxError: Unexpected token "" in shell:1

after

>> JSON.parse(Error()
SyntaxError: Unexpected token "" in
xeioex picture xeioex on 14 Feb 2019

@xeioex

  1. Fixed nxt_file_name() and style.
    https://gist.github.com/hongzhidao/a7c4af14fd269303d1abc04a5dbd81fa
    (need to refactor for supporting module)

  2. Updated module.
    https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d

tests are missing.

It seems 'hg diff' doesn't show the newly added file.

hongzhidao picture hongzhidao on 15 Feb 2019
👍2

@hongzhidao

Again, thank you for contribution.

2. Updated module.
https://gist.github.com/hongzhidao/63ae504223b4a1643b35a8883e3a985d

What about non-native modules lookup procedure? I think it is important enough to add to the patch. What else is missing (from the original plan)?

So we can start to test it as a whole.

xeioex picture xeioex on 15 Feb 2019
👍1

@xeioex

What about non-native modules lookup procedure?

Not supported yet, it's needed.

What else is missing (from the original plan)?

I'll rearrang funciton list, and it needs you and @drsm check again.

So we can start to test it as a whole.

Agree.

BTW, now the implementation seems no problem as a whole?

hongzhidao picture hongzhidao on 16 Feb 2019

@hongzhidao

BTW, now the implementation seems no problem as a whole?

Yes. I like the current design. So we can work on improving it. Once all the requirements for the original plan more or less are ready @igorsysoev will also review it before committing.

xeioex picture xeioex on 16 Feb 2019
👍1

@xeioex @hongzhidao

What about non-native modules lookup procedure? I think it is important enough to add to the patch. What else is missing (from the original plan)?

Also, there is sandboxing stuff. Should we limit access to filesystem in this case?

drsm picture drsm on 16 Feb 2019

@xeioex @hongzhidao

What about non-native modules lookup procedure? I think it is important enough to add to the patch. What else is missing (from the original plan)?

Also, there is sandboxing stuff. Should we limit access to filesystem in this case?

Good suggestion, I think it's needed. In sandbox mode, it'll behave like this.

# sandboxing

njs_test {
    {"var fs = require('fs')\r\n"
     "Error: Cannot find module \"fs\"\r\n"}
} "-s"

njs_test {
    {"var crypto = require('crypto')\r\n"
     "undefined\r\n"}
} "-s"

njs_test {
    {"var fs = require('./non-native-module.js')\r\n"
     "Error: Cannot find module \"./non-native-module.js\"\r\n"}
} "-s"
hongzhidao picture hongzhidao on 16 Feb 2019
👍1

deleted

drsm picture drsm on 16 Feb 2019

@xeioex

  1. Fixed nxt_file_name() and style.
    https://gist.github.com/hongzhidao/a7c4af14fd269303d1abc04a5dbd81fa
    (need to refactor for supporting module)

I reworked a bit.
https://gist.github.com/hongzhidao/a7c4af14fd269303d1abc04a5dbd81fa

The 'lexer' and 'generator' need unify. Two ways.

  1. Heap: x = malloc(); used; free(x);
  2. Stack: used;

I prefer the 2rd way since lexer and generator are temporarily invoked.

hongzhidao picture hongzhidao on 16 Feb 2019

@hongzhidao

2. Stack: used;

Looks good.

xeioex picture xeioex on 16 Feb 2019

@hongzhidao just a side note... is there any difference between NJS_SCOPE_GLOBAL vs
NULL checks?

drsm picture drsm on 16 Feb 2019

@drsm

  1. I refactored out njs_parser_global_scope for codes related to module.
  2. NULL. see the comment. I didn't change it and has not deepened it yet.
    It's better to ask @xeioex to explain the purpose. As my understand.
    The topmost scope is NJS_SCOPE_GLOBAL, it should exist.
    The variable reference needn't search the global scope, it should stop at global scope and module (new added).

The two places are independent of each other.

hongzhidao picture hongzhidao on 16 Feb 2019

@hongzhidao thanks!
so, the global scope has two invariants:
sc->type == NJS_SCOPE_GLOBAL
and
sc->parent == NULL

drsm picture drsm on 16 Feb 2019

@drsm

so, the global scope has two invariants: sc->type == NJS_SCOPE_GLOBAL and sc->parent == NULL

Yes. they are more or less interchangeable, but I would prefer to compare with NJS_SCOPE_GLOBAL for readability.

xeioex picture xeioex on 16 Feb 2019
👍2

@xeioex @drsm

Supported definite function list:

  1. introduced import/export statement. import x from y; export default {object}
  2. import native and non-native module. import fs from 'fs'; import lib from 'absolute/relative-path'
  3. limit global variable access in modules.
  4. limit return statement in module top-scope.
  5. restrict export statement in module top-scope.
  6. import statement is hoisted and export statement is sunk.
  7. non-native modules lookup procedure.
  8. limit access to filesystem in sandbox mode for non-native modules.
  9. besides..

Unsupported function list yet.

  1. besides..

Unclear function lists.

  1. check the duplicate name, import name is regarded as variable.
  2. error in open/fstat failed, now I reference nodejs. So I didn't throw error while open/fstat failed.
    I plan to improve error information as we discussed all the nodes should record line number after the feature, so this can be considered together.
  3. introduced njs_vmcode_export for disassembling.
  4. besides..

Unit tests.

  1. simple passed case.

Patch: (module-beta.patch)
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

My thoughts.

  1. Keep the comment and improve what I reserverd places 'besides..'.
  2. I'll follow-up module patch in 'Patch' section.
hongzhidao picture hongzhidao on 16 Feb 2019
👍2

2. limit access to filesystem in sandbox mode for non-native modules.

I think, in sandbox mode, we can simply forbid going up to parent directory.

import m1 from 'path/m1' // OK
import m2 from '../path/m2' // not OK

Because from part should be plain string (not evaluated in runtime), it is easy to look for .. in it.

xeioex picture xeioex on 16 Feb 2019

@xeioex

  1. limit access to filesystem in sandbox mode for non-native modules.

I think, in sandbox mode, we can simply forbid going up to parent directory.

import m1 from 'path/m1' // OK
import m2 from '../path/m2' // not OK

Because from part should be plain string (not evaluated in runtime), it is easy to look for .. in it.

Take a look again, both functions are supported just now.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

hongzhidao picture hongzhidao on 16 Feb 2019

by creating a module which imports itself i hit the following error:
SyntaxError: The maximum function nesting level is "5" in xxx.js:1.
the same error is fired when there is import loop.
also, the same happens when length of an import chain exceeds 5.

should we fix error messages there or it does not worth effort?

drsm picture drsm on 17 Feb 2019

@drsm

by creating a module which imports itself i hit the following error:
SyntaxError: The maximum function nesting level is "5" in xxx.js:1.
the same error is fired when there is import loop.
also, the same happens when length of an import chain exceeds 5.

should we fix error messages there or it does not worth effort?

Yes, It's an issue. I simply forbid the usage now until we are sure this usage is meaningful.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

cat /tmp/test.js && ./build/njs /tmp/test.js

import x from '/tmp/test.js'
SyntaxError: Cannot import itself "/tmp/test.js" in test.js:2

@xeioex @drsm please focus on the topmost comment of this ticket.

hongzhidao picture hongzhidao on 17 Feb 2019
👍1

@hongzhidao

  1. Fixed nxt_file_name() and style.
    https://gist.github.com/hongzhidao/a7c4af14fd269303d1abc04a5dbd81fa

While reviewing it. I decided to rewrite function from scratch. The idea is to make it similar to 'basename' shell command.

Take a look: https://gist.github.com/xeioex/27677f8ced0dbbc41349653e8397eb66

xeioex picture xeioex on 19 Feb 2019
👍1

@xeioex
What about this?

  1. Add comment for readability.
diff -r b4e164c72ad2 nxt/nxt_file.c
--- a/nxt/nxt_file.c    Tue Feb 19 16:42:02 2019 +0300
+++ b/nxt/nxt_file.c    Tue Feb 19 22:26:34 2019 +0800
@@ -27,11 +27,11 @@ nxt_basename(const char *path, nxt_str_t
         return;
     }

-    /* Trailing slashes. */
-
     end = path + length;
     p = end - 1;

+    /* Strip trailing slashes. */
+
     while (p >= path && *p == '/') { p--; }

     if (p == path - 1) {
@@ -42,6 +42,8 @@ nxt_basename(const char *path, nxt_str_t

     end = p + 1;

+    /* Remove suffix. */
+
     while (p >= path && *p != '/') { p--; }

     p++;
  1. How about renaming as nxt_file_basename?
hongzhidao picture hongzhidao on 19 Feb 2019
👍1

@hongzhidao

+void nxt_file_resolve(nxt_str_t *cwd, nxt_str_t *file, nxt_str_t *path);

I suggest to have two functions:
1) nxt_file_basename(const nxt_str_t *path, nxt_str_t *name) basename
2) nxt_file_dirname(const nxt_str_t *path, nxt_str_t *dir) dirname

Will do.

xeioex picture xeioex on 19 Feb 2019
👍1

@hongzhidao

Take a look: https://gist.github.com/xeioex/4f4ab80ff7286324332c69cb04dae3fa

After some experiments I decided to mimic the behaviour of os.path.dirname() and os.path.basename() from python, because, to me, it looks more logical.

n [118]: os.path.dirname("///A////")
Out[118]: '///A'

In [137]: os.path.dirname("p1/p2/name")
Out[137]: 'p1/p2'

In [138]: os.path.dirname("/p1/p2/name")
Out[138]: '/p1/p2'

In [139]: os.path.dirname("/p1/p2/name/")
Out[139]: '/p1/p2/name'

In [88]: os.path.basename("")                                                                                                           
Out[88]: ''

In [89]: os.path.basename("/")
Out[89]: ''

In [90]: os.path.basename("/a")                                                                                                         
Out[90]: 'a'

In [91]: os.path.basename("////")                                                                                                       
Out[91]: ''

In [92]: os.path.basename("////a/")
Out[92]: ''

In [98]: os.path.basename("path/name")                                                                                                  
Out[98]: 'name'

In [99]: os.path.basename("/path/name/")                                                                                                
Out[99]: ''
xeioex picture xeioex on 19 Feb 2019

@xeioex
Looks good, I'll deep into it later.

hongzhidao picture hongzhidao on 19 Feb 2019

@xeioex
I reworked to make dirname more generic and add more tests.

nxt_file_dirname(const nxt_str_t *path, nxt_str_t *name)
{
    const u_char  *p, *end;

    if (path->length == 0) {
        *name = nxt_string_value("");
        return;
    }

    /* Stripping basename. */

    p = path->start + path->length - 1;

    while (p >= path->start && *p != '/') { p--; }

    end = p + 1;

    if (end == path->start) {
        *name = nxt_string_value("");
        return;
    }

    /* Stripping trailing slashes. */

    while (p >= path->start && *p == '/') { p--; }

    p++;

    if (p == path->start) {
        p = end;
    }

    name->start = path->start;
    name->length = p - path->start;
}

diff -r 0edc20c86e02 njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c  Tue Feb 19 21:05:19 2019 +0300
+++ b/njs/test/njs_unit_test.c  Wed Feb 20 12:27:00 2019 +0800
@@ -12062,6 +12062,7 @@ nxt_file_dirname_test(njs_vm_t * vm, nxt
         { nxt_string("a//"),         nxt_string("a") },
         { nxt_string("p1/p2/name"),   nxt_string("p1/p2") },
         { nxt_string("/p1/p2/name"),  nxt_string("/p1/p2") },
+        { nxt_string("/p1/p2///name"),  nxt_string("/p1/p2") },
         { nxt_string("/p1/p2/name/"), nxt_string("/p1/p2/name") },
     };
hongzhidao picture hongzhidao on 20 Feb 2019
👍1

@hongzhidao

I reworked to make dirname more generic and add more tests.

It looks much better, agree.

  1. Fixed nxt_file_name() and style.
    https://gist.github.com/hongzhidao/a7c4af14fd269303d1abc04a5dbd81fa

I split style patch into distinct changes, take a look:
https://gist.github.com/xeioex/d606744087f419e357fc0dc5b6850f86

xeioex picture xeioex on 20 Feb 2019
👍2

@xeioex

Can you add this toghther?

diff -r 03be823cd95b njs/njs.c
--- a/njs/njs.c Tue Feb 12 18:56:04 2019 +0300
+++ b/njs/njs.c Wed Feb 20 21:33:00 2019 +0800
@@ -218,10 +218,11 @@ njs_vm_destroy(njs_vm_t *vm)
 nxt_int_t
 njs_vm_compile(njs_vm_t *vm, u_char **start, u_char *end)
 {
-    nxt_int_t         ret;
-    njs_lexer_t      *lexer;
-    njs_parser_t     *parser, *prev;
-    njs_generator_t  *generator;
+    nxt_int_t           ret;
+    njs_lexer_t         lexer;
+    njs_parser_t        *parser, *prev;
+    njs_generator_t     *generator;
+    njs_parser_scope_t  *scope;

     parser = nxt_mp_zalloc(vm->mem_pool, sizeof(njs_parser_t));
     if (nxt_slow_path(parser == NULL)) {
@@ -235,17 +236,15 @@ njs_vm_compile(njs_vm_t *vm, u_char **st
     prev = vm->parser;
     vm->parser = parser;

-    lexer = nxt_mp_zalloc(vm->mem_pool, sizeof(njs_lexer_t));
-    if (nxt_slow_path(lexer == NULL)) {
-        return NJS_ERROR;
-    }
+    parser->lexer = &lexer;

-    parser->lexer = lexer;
-    lexer->start = *start;
-    lexer->end = end;
-    lexer->line = 1;
-    lexer->file = vm->options.file;
-    lexer->keywords_hash = vm->shared->keywords_hash;
+    nxt_memzero(&lexer, sizeof(njs_lexer_t));
+
+    lexer.start = *start;
+    lexer.end = end;
+    lexer.line = 1;
+    lexer.file = vm->options.file;
+    lexer.keywords_hash = vm->shared->keywords_hash;

     if (vm->backtrace != NULL) {
         nxt_array_reset(vm->backtrace);
@@ -258,7 +257,9 @@ njs_vm_compile(njs_vm_t *vm, u_char **st
         goto fail;
     }

-    ret = njs_variables_scope_reference(vm, parser->scope);
+    scope = parser->scope;
+
+    ret = njs_variables_scope_reference(vm, scope);
     if (nxt_slow_path(ret != NXT_OK)) {
         goto fail;
     }
@@ -280,7 +281,7 @@ njs_vm_compile(njs_vm_t *vm, u_char **st

     nxt_memzero(generator, sizeof(njs_generator_t));

-    ret = njs_generate_scope(vm, generator, parser->scope);
+    ret = njs_generate_scope(vm, generator, scope);
     if (nxt_slow_path(ret != NXT_OK)) {
         goto fail;
     }
@@ -290,7 +291,7 @@ njs_vm_compile(njs_vm_t *vm, u_char **st
     vm->global_scope = generator->local_scope;
     vm->scope_size = generator->scope_size;

-    vm->variables_hash = parser->scope->variables;
+    vm->variables_hash = scope->variables;

     if (vm->options.init) {
         ret = njs_vm_init(vm);

I'll also need to do this while introducing module :)
BTW, the others look good.

hongzhidao picture hongzhidao on 20 Feb 2019

@hongzhidao

Final version: https://gist.github.com/xeioex/b763420e36210ee381a9d3d9fd3206b3

xeioex picture xeioex on 20 Feb 2019

@xeioex Great improvements. No problem.

hongzhidao picture hongzhidao on 20 Feb 2019

@xeioex just found that njs_benchmark is broken now, not sure is this related to the last fixxes...

drsm picture drsm on 20 Feb 2019

@drsm
It works well for me. Check it again, please.

make clean && CFLAGS='-O0 -DNXT_DEBUG_MEMORY=1 -fsanitize=address' ./configure && make test
hongzhidao picture hongzhidao on 20 Feb 2019

@hongzhidao it compiles well, but does not work

$ build/njs_benchmark v
failed: "null" vs "undefined"
$ build/njs_benchmark n
njs_vm_compile() failed
drsm picture drsm on 20 Feb 2019
👍1

@drsm I'm on it.

hongzhidao picture hongzhidao on 20 Feb 2019
👍1

@drsm @xeioex
It's an issue that existed before.

Patch
https://gist.github.com/hongzhidao/b2afd165cd01397a5254203ac9d80324

hongzhidao picture hongzhidao on 20 Feb 2019
👍2

@hongzhidao it works now, thanks!

drsm picture drsm on 20 Feb 2019

@hongzhidao

It seems, that after making njs_lexer_t local, this places should be rewritten
https://github.com/nginx/njs/blob/master/njs/njs_regexp.c#L380

    if (vm->parser != NULL) {                                                                                                    
        njs_syntax_error(vm, "%*s in %uD", p - start, start,                                                                     
                         vm->parser->lexer->line);                                                                               

    } else {                                                                                                                     
        njs_syntax_error(vm, "%*s", (int) (p - start, start);                                                                    
    }
xeioex picture xeioex on 20 Feb 2019

@xeioex
I'm not familiar with vm->trace, where will it be called? Can be outside of njs_vm_compile()?

hongzhidao picture hongzhidao on 20 Feb 2019

@xeioex

Are these typos in njs_value_t?

diff -r 736cfe916b2b njs/njs_vm.h
struct {
-    njs_value_type_t              type:8;  /* 6 bits */
+    njs_value_type_t              type:8;  /* 8 bits */
     ...
}
hongzhidao picture hongzhidao on 21 Feb 2019

@hongzhidao no

    NJS_OBJECT_VALUE          = 0x20,
#define NJS_TYPE_MAX         (NJS_OBJECT_VALUE + 1)
} njs_value_type_t;

so, as for now, 6 bits of 8 are used

drsm picture drsm on 21 Feb 2019
👍2

@hongzhidao

Please, update your patch with the recent commits in the repo, so we can start look at it again.

xeioex picture xeioex on 21 Feb 2019

@xeioex I’m so busy today, you will wait until tomorrow:)

hongzhidao picture hongzhidao on 21 Feb 2019
👍1

@xeioex

Take a look, please.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

I'll see your feedback tomorrow.

hongzhidao picture hongzhidao on 21 Feb 2019
👍2

@hongzhidao

Great job! Looks like a solid beta.

Unfortunately I do not have enough time for deep review before njs release (26.02).

Minor improvements:

1) https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726#file-module-beta-patch-L776

Maybe this change should be a separate patch.

2) https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726#file-module-beta-patch-L1416
and similar places
{hash: hash} -> {hash} ?

3) https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726#file-module-beta-patch-L15
and similar places
#include <njs_module.h> can be added to njs_core.h

xeioex picture xeioex on 22 Feb 2019
👍1

@hongzhidao
njs_internal_error looks better, but it will be hidden by this one

typo?

drsm picture drsm on 22 Feb 2019
👍1

@xeioex @drsm

Patches before introducing module:

  1. Improved parser syntax error.
  2. Improved parser scope filename.
    https://gist.github.com/hongzhidao/f54fd6be1f84ff665aa7379b7ef2bd4c

Updated module-patch.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

Another optional patch.
Make njs_value_index() clearer.
https://gist.github.com/hongzhidao/3d72c533161a56dc5236e1f563f6fd77

hongzhidao picture hongzhidao on 23 Feb 2019

@hongzhidao 

# HG changeset patch
# User Dmitry Volyntsev <[email protected]>
# Date 1551116362 -10800
#      Mon Feb 25 20:39:22 2019 +0300
# Node ID 1120ca281ed9d399829186910e74d888298039fa
# Parent  d498378197210c8f226f32018031f054150785e5
Improved checking filename is not empty.

file->start can be non NULL for empty string.

diff --git a/njs/njs_parser.c b/njs/njs_parser.c
--- a/njs/njs_parser.c
+++ b/njs/njs_parser.c
@@ -233,7 +233,7 @@ njs_parser_scope_begin(njs_vm_t *vm, njs
     scope->values[0] = values;
     scope->values[1] = NULL;

-    if (parser->lexer->file.start != NULL) {
+    if (parser->lexer->file.length != 0) {
         ret = njs_name_copy(vm, &scope->file, &parser->lexer->file);
         if (nxt_slow_path(ret != NXT_OK)) {
             return NXT_ERROR;
@@ -2758,7 +2758,7 @@ njs_parser_trace_handler(nxt_trace_t *tr
     if (vm->parser != NULL) {
         lexer = vm->parser->lexer;

-        if (lexer->file.start != NULL) {
+        if (lexer->file.length != 0) {
             njs_internal_error(vm, "%s in %V:%uD", start, &lexer->file,
                                lexer->line);
         } else {
@@ -2795,7 +2795,7 @@ njs_parser_scope_error(njs_vm_t *vm, njs
         p = end - width;
     }

-    if (file->start != NULL) {
+    if (file->length != 0) {
         p = nxt_sprintf(p, end, " in %V:%uD", file, line);

     } else {
xeioex picture xeioex on 25 Feb 2019
👍1

@xeioex

improvements.
https://gist.github.com/hongzhidao/68bde435c3c34661274ff72ab3b25bd9

modules support.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

hongzhidao picture hongzhidao on 26 Feb 2019

@hongzhidao

improvements.
https://gist.github.com/hongzhidao/68bde435c3c34661274ff72ab3b25bd9

https://gist.github.com/hongzhidao/68bde435c3c34661274ff72ab3b25bd9#file-njs-improvements-01-patch-L144

drsm picture drsm on 26 Feb 2019
👍1

@drsm @xeioex

Updated again.

improvements.
https://gist.github.com/hongzhidao/68bde435c3c34661274ff72ab3b25bd9

modules support.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

hongzhidao picture hongzhidao on 26 Feb 2019
👍1

@hongzhidao

Here: https://gist.github.com/hongzhidao/68bde435c3c34661274ff72ab3b25bd9#file-njs-improvements-01-patch-L264

You removed type check for string type. Because we insert into vm->values_hash only primitive types (primitives are covered with the first comparison) and strings?

If so, we need a commentary here (will do).

xeioex picture xeioex on 27 Feb 2019

@xeioex

See njs_value_index() first.

if (src->type != NJS_STRING || src->short_string.size != NJS_STRING_LONG) {
    size = sizeof(njs_value_t);
    start = (u_char *) src;
} else {
   size = src->long_string.size;
   start = src->long_string.data->start;
}

It means in njs_values_hash_test().

if (lhq->key.length == sizeof(njs_value_t)) {
     start = (u_char *) value;
} else {
    start = value->long_string.data->start;
}

You removed type check for string type. Because we insert into vm->values_hash only primitive types (primitives are covered with the first comparison) and strings?

Yes. And there are already some commentary before njs_value_index(). Help add.

/*
 * Constant values such as njs_value_true are copied to values_hash during
 * code generation when them are used as operands to guarantee aligned value.
 */

BTW, you forgot one place introduced in
http://hg.nginx.org/njs/rev/7345fd14d1b4
https://gist.github.com/hongzhidao/68bde435c3c34661274ff72ab3b25bd9#file-njs-improvements-01-patch-L149

And style.

diff -r 7345fd14d1b4 njs/njs_string.c
--- a/njs/njs_string.c  Tue Feb 26 17:30:02 2019 +0800
+++ b/njs/njs_string.c  Thu Feb 28 01:18:19 2019 +0800
@@ -4349,7 +4349,7 @@ static const nxt_lvlhsh_proto_t  njs_val

 /*
  * Constant values such as njs_value_true are copied to values_hash during
- * code generation when them are used as operands to guarantee aligned value.
+ * code generation when they are used as operands to guarantee aligned value.
  */
hongzhidao picture hongzhidao on 27 Feb 2019
👍1

@hongzhidao

https://gist.github.com/xeioex/5faab0e5238a32771d5773c4ab3960fc

https://gist.github.com/xeioex/5faab0e5238a32771d5773c4ab3960fc#file-improvements3-patch-L170
missed replacement:

-    if (start != (u_char *) src) {
+    if (long_string) {
  1. BTW, it seems, cwd is also is needed here: https://gist.github.com/hongzhidao/68bde435c3c34661274ff72ab3b25bd9#file-njs-improvements-01-patch-L159

will do.

xeioex picture xeioex on 27 Feb 2019

@xeioex

https://gist.github.com/xeioex/5faab0e5238a32771d5773c4ab3960fc#file-improvements3-patch-L170
missed replacement:

I think it's better to remain unchanged. See the commentory, I prefer start != src.

     if (start != (u_char *) src) {
        /*
         * The source node value must be updated with the shared value
         * allocated from the permanent memory pool because the node
         * value can be used as a variable initial value.
         */
        *(njs_value_t *) src = *value;
    }

BTW, it seems, cwd is also is needed here:

It's better to process it in module patch. I'll think of it.

Don't forget this.

diff -r 7345fd14d1b4 njs/njs_string.c
--- a/njs/njs_string.c  Tue Feb 26 17:30:02 2019 +0800
+++ b/njs/njs_string.c  Thu Feb 28 01:18:19 2019 +0800
@@ -4349,7 +4349,7 @@ static const nxt_lvlhsh_proto_t  njs_val

 /*
  * Constant values such as njs_value_true are copied to values_hash during
- * code generation when them are used as operands to guarantee aligned value.
+ * code generation when they are used as operands to guarantee aligned value.
  */

Others are no problem.

hongzhidao picture hongzhidao on 27 Feb 2019

@hongzhidao

take a look:

# HG changeset patch
# User Dmitry Volyntsev <[email protected]>
# Date 1551290334 -10800
#      Wed Feb 27 20:58:54 2019 +0300
# Node ID 64f63b3638733c42b044c017fb89540cdbc452d2
# Parent  4276436234aa06ca678ab9fdac3dbaa3b39245b3
Shell: passing original filename to parser.

diff --git a/njs/njs_shell.c b/njs/njs_shell.c
--- a/njs/njs_shell.c
+++ b/njs/njs_shell.c
@@ -14,6 +14,7 @@
 #include <fcntl.h>
 #include <stdlib.h>
 #include <sys/stat.h>
+#include <sys/param.h>
 #include <locale.h>

 #include <readline.h>
@@ -195,6 +196,7 @@ static njs_console_t  njs_console;
 int
 main(int argc, char **argv)
 {
+    char          cwd[MAXPATHLEN];
     nxt_int_t     ret;
     njs_opts_t    opts;
     njs_vm_opt_t  vm_options;
@@ -215,14 +217,14 @@ main(int argc, char **argv)
     nxt_memzero(&vm_options, sizeof(njs_vm_opt_t));

     if (!opts.quiet) {
-        if (opts.file != NULL) {
-            vm_options.file.start = (u_char *) opts.file;
-            vm_options.file.length = strlen(opts.file);
-            nxt_file_basename(&vm_options.file, &vm_options.file);
+        if (opts.file == NULL) {
+            getcwd(cwd, sizeof(cwd));
+            memcpy(cwd + strlen(cwd), "/shell", sizeof("/shell"));
+            opts.file = cwd;
+        }

-        } else {
-            vm_options.file = nxt_string_value("shell");
-        }
+        vm_options.file.start = (u_char *) opts.file;
+        vm_options.file.length = strlen(opts.file);
     }

     vm_options.init = !opts.interactive;
xeioex picture xeioex on 27 Feb 2019
👍1

@xeioex @drsm

Updated module patch based on the latest commit.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

Sorry forgot to separate Shell: passing original filename to parser..
It's too late that I'm late 5 hours in Russia :)

hongzhidao picture hongzhidao on 27 Feb 2019
👍1

@hongzhidao

Regarding: https://github.com/nginx/njs/issues/91#issuecomment-465672984

https://gist.github.com/xeioex/b7d0d6472f002cc1f04f8207bbd625ce

Once lexer is allocated on stack, it can be unavailable later.

xeioex picture xeioex on 28 Feb 2019

@xeioex

The latest version of module patch.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

hongzhidao picture hongzhidao on 28 Feb 2019

@hongzhidao

Regarding: #91 (comment)

https://gist.github.com/xeioex/b7d0d6472f002cc1f04f8207bbd625ce

Once lexer is allocated on stack, it can be unavailable later.

Get it.

BTW, I'm wondering when to call trace->handler, and what does trace exactly act on (what's its role)?

hongzhidao picture hongzhidao on 28 Feb 2019

BTW, I'm wondering when to call trace->handler, and what does trace exactly act on (what's its role)?

It is rarely used. It is a way to catch errors from libraries like prce, where vm is unavailable (because nxt code does not know anything about njs structs).

nxt/nxt_pcre.c:

nxt_int_t                                                                                                                        
nxt_regex_compile(nxt_regex_t *regex, u_char *source, size_t len,                                                                
    nxt_uint_t options, nxt_regex_context_t *ctx) 
...
            nxt_alert(ctx->trace, NXT_LEVEL_ERROR,                                                                               
                      "pcre_compile(\"%s\") failed: %s at \"%s\"",                                                               
                      pattern, errstr, error);

nxt_alert() would pass an error to a chain of trace handlers. Finally, the last trace handler makes vm exception out of it. I prefer to avoid it (if possible) due to its limitations.

Before calling library function, a handler is installed which catches nxt_alert() invocations

njs_regexp_pattern_compile(njs_vm_t *vm, nxt_regex_t *regex, u_char *source,                                                     
    int options) 
    handler = vm->trace.handler;                                                                                                 
    vm->trace.handler = njs_regexp_compile_trace_handler;                                                                        

    /* Zero length means a zero-terminated string. */                                                                            
    ret = nxt_regex_compile(regex, source, 0, options, vm->regex_context);                                                       

    vm->trace.handler = handler;                                                                                                 

    if (nxt_fast_path(ret == NXT_OK)) {                                                                                          
        return regex->ncaptures;                                                                                                 
    }

The original idea: each trace handler would output to the log its part (without copying). But, it is not suitable for JavaScript when all the errors should be available as Exception values (and exceptions can be stored as temporary values).

xeioex picture xeioex on 28 Feb 2019
👍1

@hongzhidao

Improved version: https://gist.github.com/xeioex/2cf233c9d2e532c3cd794362b8d21553

1) expect tests added and improved
2) -p option can be used multiple times
3) NJS_PATH env variable is supported
4) njs/test/module/ code is improved (; added)

xeioex picture xeioex on 1 Mar 2019
👍1

@hongzhidao

looking at 

./build/njs -p njs/test/module/
interactive njs 0.3.0

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> import lib from 'lib1.js'
SyntaxError: Cannot find module "lib1.js" in shell:1

https://gist.github.com/xeioex/2cf233c9d2e532c3cd794362b8d21553#file-modules-01-03-patch-L324

I think, by default relative should be true. It is only false if the first character is /.

xeioex picture xeioex on 1 Mar 2019

@hongzhidao 

cat ./njs/test/module/recursive.js
import lib from './recursive.js';

./build/njs -p njs/test/module/
...
>> import lib from './recursive.js'
SyntaxError: The maximum function nesting level is "5" in recursive.js:1
xeioex picture xeioex on 1 Mar 2019

@xeioex

Thanks for your improvements, review again please.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

Note:

  1. There is big change of searching module. It should be non-native module which we can't find .
  2. Still simply avoid importing self module.

BTW: The questions about nginx module.

  1. Should we need to consider support module in nginx module now?
  2. Does nginx module plan to support phases like post_read, access, header/body filter in short term?
hongzhidao picture hongzhidao on 2 Mar 2019

Should we need to consider support module in nginx module now?

I think this should be a separate patch (Will do).

Does nginx module plan to support phases like post_read, access in short term?

We prefer to limit the number of directives at minimum. Most of the time js_content is enough (if you want js_access for example, you can use auth_request + js_conent). We can consider adding new phase handler if it is really needed.

header/body filter in short term

yes, but not in the short term.

xeioex picture xeioex on 2 Mar 2019

@xeioex
In real case, it's common that do authentication at access phase without changing content. Such as:

location /test {
       js_access  auth;
       root  html;
       # proxy_pass  upstream;
}

Now, we can't do it in njs?
And I believe developers would like to use UNIT as application server, it'll do the same thing like js_content.

hongzhidao picture hongzhidao on 2 Mar 2019

@hongzhidao

the idea is: js_content can be used in many combinations.
For example:
auth_request + js_content == js_access.

xeioex picture xeioex on 2 Mar 2019

@xeioex I get it !!!

hongzhidao picture hongzhidao on 2 Mar 2019

@hongzhidao
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726

https://gist.github.com/xeioex/2422edac1bb1b157a84d306805337adf

Some fixes and improvements in njs_module.c
1) style, indentation
2) simplification is calculating file.length in njs_module_absolute_path() and njs_module_relative_path()
3) njs_parser_module() improved,
a) fixed fd leak for recursive modules (fd leaks are important, because njs lives inside nginx worker processes, and can exhaust available fds).
b) check for empty name
4) a better name for njs_file_check_same()

xeioex picture xeioex on 5 Mar 2019
👍1

@hongzhidao

https://gist.github.com/xeioex/2e2bab9178dea495b1cdd8554cb17908

Some fixes and improvements in njs_parser.c, more tests

Also found an issue

./build/njs -p njs/test/module/
...
>> import lib1 from 'lib1.js'
undefined
>> lib1.inc()
undefined
>> lib1.get()
1
>> import lib11 from 'lib1.js'
undefined
>> lib11.get()
0

Objects returned by the first and second imports are not the same. On it.

xeioex picture xeioex on 6 Mar 2019

@xeioex great work.

Some fixes and improvements in njs_parser.c

Show related lines in gist to issues here, please.

hongzhidao picture hongzhidao on 6 Mar 2019

Show related lines in gist to issues here, please.

  1. new functions moved to default branch in njs_parser_statement() (to avoid duplication of handling ';', '}' and END token)
    https://gist.github.com/xeioex/2e2bab9178dea495b1cdd8554cb17908#file-modules-03-06-patch-L745

  2. njs_parser_import_statement() simplified (avoid handling tokens which can be handled by njs_parser_statement())

  3. added test module with a changeable state
    https://gist.github.com/xeioex/2e2bab9178dea495b1cdd8554cb17908#file-modules-03-06-patch-L1376

  4. more import syntax test
    https://gist.github.com/xeioex/2e2bab9178dea495b1cdd8554cb17908#file-modules-03-06-patch-L1561

xeioex picture xeioex on 7 Mar 2019
👍1

@hongzhidao

https://gist.github.com/xeioex/c73e9db29579e74cf90df18cf145f3d6

  1. More tests for return, export instructions in modules.
  2. User-friendly exception for non-default export ("Non-default export is not supported").
  3. some style fixes.
  4. fix for njs_module_load() in shell mode. To prevent loading modules multiple times, for each command.
+ if (vm->options.accumulative) {
+        nxt_array_reset(vm->modules);
+ }

I am done with review. Next week I plan to add nginx related patches, for adding module paths.

xeioex picture xeioex on 7 Mar 2019
👍2

Next week I plan to add nginx related patches, for adding module paths.

js_content module::func; // consider this format, I'm not sure yet.

hongzhidao picture hongzhidao on 8 Mar 2019

@xeioex
patch https://gist.github.com/xeioex/c73e9db29579e74cf90df18cf145f3d6

a/njs/njs_parser.h b/njs/njs_parser.h
--- a/njs/njs_parser.h
+++ b/njs/njs_parser.h
@@ -203,6 +203,10 @@ typedef enum {
     NJS_TOKEN_SET_IMMEDIATE,
     NJS_TOKEN_CLEAR_TIMEOUT,

+    NJS_TOKEN_IMPORT,
+    NJS_TOKEN_FROM,
+    NJS_TOKEN_EXPORT,
+
     NJS_TOKEN_RESERVED,
 } njs_token_t;

njs_token_t for njs_lexer.h

and 

njs/njs_module.c: In function ‘njs_parser_module’:
njs/njs_module.c:92:26: error: ‘njs_lexer_t’ has no member named ‘text’
     name = &parser->lexer->text;
                          ^
make[2]: *** [build/njs_module.o] Error 1

cat njs/njs_lexer.h

typedef struct {
    njs_lexer_token_t               *lexer_token;

    u_char                          *prev_start;
    njs_token_t                     prev_token:16;

    uint8_t                         property;      /* 1 bit */
    njs_token_t                     property_token:16;

    uint32_t                        line;
    nxt_str_t                       file;

    nxt_lvlhsh_t                    keywords_hash;

    u_char                          *start;
    u_char                          *end;
} njs_lexer_t;
macoomac picture macoomac on 8 Mar 2019

@macoomac

yes, patch is broken a bit after https://github.com/nginx/njs/commit/7511f75e2bcc8adc37109702523857de94df0ff5, will be fixed.

xeioex picture xeioex on 8 Mar 2019

@macoomac

Please, try the updated patch:
https://gist.github.com/xeioex/4ba5d3eb807da643fb63b9895aadd366

xeioex picture xeioex on 8 Mar 2019

@xeioex
thanks!

macoomac picture macoomac on 8 Mar 2019

I am done with review. Next week I plan to add nginx related patches, for adding module paths.

Still needs a bit work.

Refactored njs_parser_function_lambda().
https://gist.github.com/hongzhidao/75b0ac9e0a7bc0026fb1e1e40679f5a7

hongzhidao picture hongzhidao on 10 Mar 2019
👍1

@hongzhidao

Updated patch + disassembler improments:
https://gist.github.com/xeioex/6ca88af23ca13c966691fc2ba985680a

njs -p njs/test/module/libs/ -d ./njs/test/module/normal.js

lib1.js:hash
00000 METHOD FRAME      0015 1C714A0 1
00040 MOVE              0002 1C714B0
00072 FUNCTION CALL     0004
00096 METHOD FRAME      0004 1C714C0 1
00136 MOVE              0002 1C714D0
00168 FUNCTION CALL     0024
00192 METHOD FRAME      0024 1C714E0 1
00232 MOVE              0002 1C714F0
00264 FUNCTION CALL     0014
00288 RETURN            0014
lib1.js:inc
00000 PROPERTY GET      0014 0025 1C71500
00040 POST INC          0004 0014 0014
00080 PROPERTY SET      0014 0025 1C71500
00120 RETURN            1C71520
lib1.js:get
00000 PROPERTY GET      0004 0025 1C71500
00040 RETURN            0004
lib1.js:module
00000 OBJECT COPY       0015 0141
00032 OBJECT            0034
00056 PROPERTY SET      1C71510 0034 1C71500
00096 MOVE              0025 0034
00128 OBJECT            0034
00152 OBJECT COPY       0044 0004
00184 PROPERTY SET      0044 0034 1C71540
00224 OBJECT COPY       0044 0014
00256 PROPERTY SET      0044 0034 1C71550
00296 OBJECT COPY       0044 0024
00328 PROPERTY SET      0044 0034 1C71560
00368 RETURN            0034
hash.js:hash
00000 METHOD FRAME      0019 1C714A0 1
00040 MOVE              0002 1C714B0
00072 FUNCTION CALL     0004
00096 METHOD FRAME      0004 1C714C0 1
00136 MOVE              0002 1C714D0
00168 FUNCTION CALL     0024
00192 METHOD FRAME      0024 1C714E0 1
00232 MOVE              0002 1C714F0
00264 FUNCTION CALL     0014
00288 RETURN            0014
hash.js:module
00000 OBJECT COPY       0019 0141
00032 OBJECT            0014
00056 OBJECT COPY       0024 0004
00088 PROPERTY SET      0024 0014 1C71540
00128 RETURN            0014
sub2.js:hash
00000 METHOD FRAME      0018 1C71540 0
00040 FUNCTION CALL     0004
00064 RETURN            0004
sub2.js:module
00000 FUNCTION          0034 1C6BA80
00032 OBJECT COPY       0018 0161
00064 OBJECT            0014
00088 OBJECT COPY       0024 0004
00120 PROPERTY SET      0024 0014 1C71540
00160 RETURN            0014
sub1.js:hash
00000 METHOD FRAME      0017 1C71540 1
00040 MOVE              0002 0027
00072 FUNCTION CALL     0004
00096 RETURN            0004
sub1.js:exception
00000 OBJECT            0004
00024 PROPERTY GET      0004 0004 1C77710
00064 PROPERTY GET      0004 0004 1C77710
00104 RETURN            0004
sub1.js:module
00000 FUNCTION          0034 1C6A8C0
00032 OBJECT COPY       0017 0171
00064 OBJECT COPY       0027 0141
00096 OBJECT            0024
00120 OBJECT COPY       0034 0004
00152 PROPERTY SET      0034 0024 1C71540
00192 OBJECT COPY       0034 0014
00224 PROPERTY SET      0034 0024 1C77730
00264 RETURN            0024
lib3.js:hash
00000 METHOD FRAME      0016 1C71540 0
00040 FUNCTION CALL     0004
00064 RETURN            0004
lib3.js:exception
00000 METHOD FRAME      0016 1C77730 0
00040 FUNCTION CALL     0004
00064 RETURN            0004
lib3.js:module
00000 FUNCTION          0044 1C69400
00032 OBJECT COPY       0016 0181
00064 OBJECT            0024
00088 OBJECT COPY       0034 0004
00120 PROPERTY SET      0034 0024 1C71540
00160 OBJECT COPY       0034 0014
00192 PROPERTY SET      0034 0024 1C77730
00232 RETURN            0024
lib2.js:hash
00000 METHOD FRAME      0015 1C71540 0
00040 FUNCTION CALL     0004
00064 RETURN            0004
lib2.js:module
00000 FUNCTION          0044 1C687C0
00032 OBJECT COPY       0015 0191
00064 OBJECT            0014
00088 OBJECT COPY       0024 0004
00120 PROPERTY SET      0024 0014 1C71540
00160 RETURN            0014
normal.js:main
00000 FUNCTION          0054 1C649C0
00032 OBJECT COPY       01B1 0151
00064 FUNCTION          0034 1C68700
00096 OBJECT COPY       01C1 01A1
00128 OBJECT COPY       01D1 0151
00160 OBJECT COPY       01E1 0141
00192 METHOD FRAME      01E1 1C714A0 1
00232 MOVE              0002 1C714B0
00264 FUNCTION CALL     01F1
00288 METHOD FRAME      01F1 1C714C0 1
00328 MOVE              0002 1C714D0
00360 FUNCTION CALL     0211
00384 METHOD FRAME      0211 1C714E0 1
00424 MOVE              0002 1C714F0
00456 FUNCTION CALL     0201
00480 METHOD FRAME      01B1 1C71540 0
00520 FUNCTION CALL     0211
00544 NOT EQUAL         0211 0211 0201
00584 JUMP IF FALSE     0211 +128
00616 METHOD FRAME      1C5B120 1C77770 1
00656 MOVE              0002 1C77780
00688 FUNCTION CALL     0211
00712 METHOD FRAME      01C1 1C71540 0
00752 FUNCTION CALL     0211
00776 NOT EQUAL         0211 0211 0201
00816 JUMP IF FALSE     0211 +128
00848 METHOD FRAME      1C5B120 1C77770 1
00888 MOVE              0002 1C77780
00920 FUNCTION CALL     0211
00944 METHOD FRAME      01B1 1C71560 0
00984 FUNCTION CALL     0211
01008 NOT EQUAL         0211 0211 1C71510
01048 JUMP IF FALSE     0211 +128
01080 METHOD FRAME      1C5B120 1C77770 1
01120 MOVE              0002 1C77780
01152 FUNCTION CALL     0211
01176 METHOD FRAME      01D1 1C71560 0
01216 FUNCTION CALL     0211
01240 NOT EQUAL         0211 0211 1C71510
01280 JUMP IF FALSE     0211 +128
01312 METHOD FRAME      1C5B120 1C77770 1
01352 MOVE              0002 1C77780
01384 FUNCTION CALL     0211
01408 METHOD FRAME      01B1 1C71550 0
01448 FUNCTION CALL     0211
01472 METHOD FRAME      01B1 1C71560 0
01512 FUNCTION CALL     0211
01536 NOT EQUAL         0211 0211 1C77790
01576 JUMP IF FALSE     0211 +128
01608 METHOD FRAME      1C5B120 1C77770 1
01648 MOVE              0002 1C77780
01680 FUNCTION CALL     0211
01704 METHOD FRAME      01D1 1C71560 0
01744 FUNCTION CALL     0211
01768 NOT EQUAL         0211 0211 1C77790
01808 JUMP IF FALSE     0211 +128
01840 METHOD FRAME      1C5B120 1C77770 1
01880 MOVE              0002 1C77780
01912 FUNCTION CALL     0211
01936 METHOD FRAME      1C5B120 1C77770 1
01976 MOVE              0002 1C777A0
02008 FUNCTION CALL     0211
02032 STOP              0211

'passed!'

Please review the added patch.

BTW, while implementing the patch, found the issue with module flag

--- a/njs/njs_parser.c
+++ b/njs/njs_parser.c
@@ -2001,7 +2001,8 @@ njs_parser_module_lambda(njs_vm_t *vm, n
         return NJS_TOKEN_ERROR;
     }

-    parser->scope->module = 1;
+    node->scope = parser->scope;
+    node->scope->module = 1;
xeioex picture xeioex on 11 Mar 2019
👍2

@xeioex Great improvements without problems.

I think arrow function can be also added easily in the disassembler.

hongzhidao picture hongzhidao on 12 Mar 2019

@xeioex
Updated patch https://gist.github.com/xeioex/6ca88af23ca13c966691fc2ba985680a

:)

cat njs/njs_parser.h.rej
--- njs/njs_parser.h
+++ njs/njs_parser.h
@@ -25,12 +25,14 @@
nxt_array_t values[2]; / Array of njs_value_t. */
njs_index_t next_index[2];

+ nxt_str_t cwd;
nxt_str_t file;

 njs_scope_t                     type:8;
 uint8_t                         nesting;     /* 4 bits */
 uint8_t                         argument_closures;
 uint8_t                         arguments_object;

+ uint8_t module;
};

macoomac picture macoomac on 12 Mar 2019

@macoomac

Better format.
https://www.markdownguide.org/basic-syntax/#code

hongzhidao picture hongzhidao on 12 Mar 2019

@hongzhidao
thanks

macoomac picture macoomac on 12 Mar 2019

@macoomac @drsm

js_path directive is added:

http {
     js_path "/tmp/lib";
     js_path "/tmp/lib2";

     js_include http.js;
...

https://gist.github.com/xeioex/ac9ff2605fef81763b18289546d63d09

xeioex picture xeioex on 12 Mar 2019
👍3 😄1

@xeioex
https://gist.github.com/xeioex/ac9ff2605fef81763b18289546d63d09#file-modules-03-12-patch-L697

drsm picture drsm on 13 Mar 2019

more f.js
function add(a, b) {
return a + b;
}
export default {add}

more m.js
import add from './f.js'

console.log("ccc",add(1,2));

./build/njs /lib/m.js

TypeError: object is not a function
at console.log (native)
at main (native)

macoomac picture macoomac on 16 Mar 2019

Supported function list?
module.exports = function .. { ... }

exports.default =(...)(..);
module.exports = exports['default'];

macoomac picture macoomac on 16 Mar 2019

more f.js
function add(a, b) {
return a + b;
}
export default {add}

more m.js
import add from './f.js'

console.log("ccc",add(1,2));

./build/njs /lib/m.js

TypeError: object is not a function
at console.log (native)
at main (native)

add is an object. My suggestion:

more f.js
function add(a, b) {
return a + b;
}
export default {add}

more m.js
import f from './f.js'

console.log("ccc",f.add(1,2));
hongzhidao picture hongzhidao on 16 Mar 2019
👍1

Supported function list?
module.exports = function .. { ... }

exports.default =(...)(..);
module.exports = exports['default'];

No.

hongzhidao picture hongzhidao on 16 Mar 2019

./build/njs -p /home/work/es/ /home/work/cc.js
SyntaxError: The maximum function nesting level is "5" in esprima.js:422

macoomac picture macoomac on 16 Mar 2019

@macoomac

./build/njs -p /home/work/es/ /home/work/cc.js
SyntaxError: The maximum function nesting level is "5" in esprima.js:422

What's the content of /home/work/cc.js?

hongzhidao picture hongzhidao on 16 Mar 2019

@macoomac @hongzhidao

SyntaxError: The maximum function nesting level is "5" in esprima.js:422

looks like there is circular dependency in esprima.js, this is not yet supported.

drsm picture drsm on 16 Mar 2019

@hongzhidao @xeioex 

$ cat aa.js ab.js 
import b from 'ab.js';
import a from 'aa.js';
$ build/njs aa.js -p .
SyntaxError: The maximum function nesting level is "5" in aa.js:1
drsm picture drsm on 16 Mar 2019

@hongzhidao @xeioex @drsm

./build/njs /home/work/cc.js

SyntaxError: in 963

more cc.js
import esprima from './es/esprima.js'
var ctype="var c=2;"
console.log(esprima.parse(ctype))

more es/esprima.js
from http://esprima.org/ and https://github.com/jquery/esprima

macoomac picture macoomac on 18 Mar 2019

@macoomac

@hongzhidao @xeioex @drsm

./build/njs /home/work/cc.js

SyntaxError: in 963

more cc.js
import esprima from './es/esprima.js'
var ctype="var c=2;"
console.log(esprima.parse(ctype))

more es/esprima.js
from http://esprima.org/ and https://github.com/jquery/esprima

Thank you.
Please minimize the code while found a problem as much as possible, It'll help us debug.

Sorry that I can't reproduce the issue.

hongzhidao picture hongzhidao on 18 Mar 2019

@hongzhidao

https://gist.github.com/xeioex/573ee7cc44366ad71288c982dd3d311c

Some fixes and improvements from @igorsysoev.

Also found double-free issue on our buildbot.
https://gist.github.com/hongzhidao/3f6261bc2a73e60a47848277a47ec726#file-module-beta-patch-L411

body.start can be uninitialized here.

One more issue is present on x86 machines (debian 9).

./build/njs -p njs/test/module/libs ./njs/test/module/normal.js
ASAN:DEADLYSIGNAL
=================================================================
==4639==ERROR: AddressSanitizer: SEGV on unknown address 0xbebebf0e (pc 0x800e6644 bp 0xbfdd6938 sp 0xbfdd68e0 T0)
    #0 0x800e6643 in njs_vmcode_interpreter njs/njs_vm.c:178
    #1 0x800e50c9 in njs_vm_start njs/njs.c:590
    #2 0x800d550f in njs_process_script njs/njs_shell.c:689
    #3 0x800d4930 in njs_process_file njs/njs_shell.c:539
    #4 0x800d3409 in main njs/njs_shell.c:249
    #5 0xb6f06285 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18285)
    #6 0x800d2f90  (/home/xeioex/debian9-x86/njs-0.3.0/build/njs+0xcf90)

Some memory alignment issue, I guess.

On it. After fixing it I plan to commit the patch.

xeioex picture xeioex on 19 Mar 2019

@xeioex Great work.

hongzhidao picture hongzhidao on 20 Mar 2019

Hi @xeioex @drsm@drsm
I have two questions ;
First:When I want to use "import module" to import third party "esprima.js", the following problem occurs:

[root@NetEnc-centos7-dev-vm path-to-njs]# ./build/njs -p /home/work/dwj/es/ /home/work/dwj/cc.js
SyntaxError: in 963
[root@NetEnc-centos7-dev-vm path-to-njs]# more /home/work/dwj/cc.js
import e from './es/esprima.js'

var c = 'function eat(){};';
console.log(e.parse("c"));

The content of line 963 in the esprima.js script is a regular expression.
like this:
NonAsciiIdentifierStart: /[xAAxB5xBAxC0-xD6xD8-xF6xF8-u02C1u02C6-u02D1u02E0-u02E4u02ECu02EEu0370-u0374u0376u0377u037A,

how to solve this problem?

second: If esprima.js does not have the above problem, the prompt is as follows:
[root@NetEnc-centos7-dev-vm path-to-njs]# ./build/njs -p /home/work/dwj/es/ /home/work/dwj/cc.js
SyntaxError: export statement is required. in esprima.js:5502
[root@NetEnc-centos7-dev-vm path-to-njs]#
How should I write the "export default" statement in esprima.js?

Esprima.js script has been attached。
esprima.zip

Thank you !

dongwanjun123 picture dongwanjun123 on 21 Mar 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

reyou picture reyou  Â·  5Comments
drsm picture drsm  Â·  4Comments
porunov picture porunov  Â·  3Comments
an0ma1ia picture an0ma1ia  Â·  4Comments
laith-leo picture laith-leo  Â·  5Comments