The bedrock from digitalbazaar

`posix` fails to build on Node 10.x / posix removal

It appears that the only use for posix has to do with the logging system: https://github.com/digitalbazaar/bedrock/blob/master/lib/loggers.js#L98

How essential is the functionality? Can bedrock do without a posix dependency?

There is one error at the beginning of the log and the rest are warnings:

> [email protected] install /home/matt/dev/bedrock-dev/bedrock-passport/test/node_modules/posix
> node-gyp rebuild

make: Entering directory '/home/matt/dev/bedrock-dev/bedrock-passport/test/node_modules/posix/build'
  CXX(target) Release/obj.target/posix/src/posix.o
In file included from ../node_modules/nan/nan.h:190:0,
                 from ../src/posix.cc:1:
../node_modules/nan/nan_maybe_43_inl.h: In function ‘Nan::Maybe<bool> Nan::ForceSet(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Value>, v8::PropertyAttribute)’:
../node_modules/nan/nan_maybe_43_inl.h:88:15: error: ‘class v8::Object’ has no member named ‘ForceSet’
   return obj->ForceSet(GetCurrentContext(), key, value, attribs);
               ^~~~~~~~
In file included from ../src/posix.cc:1:0:
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Value> Nan::MakeCallback(v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*)’:
../node_modules/nan/nan.h:817:60: warning: ‘v8::Local<v8::Value> node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*)’ is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations]
         v8::Isolate::GetCurrent(), target, func, argc, argv);
                                                            ^
In file included from ../node_modules/nan/nan.h:47:0,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/node.h:172:50: note: declared here
                 NODE_EXTERN v8::Local<v8::Value> MakeCallback(
                                                  ^
/home/matt/.node-gyp/10.0.0/include/node/node.h:88:42: note: in definition of macro ‘NODE_DEPRECATED’
     __attribute__((deprecated(message))) declarator
                                          ^~~~~~~~~~
In file included from ../src/posix.cc:1:0:
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Value> Nan::MakeCallback(v8::Local<v8::Object>, v8::Local<v8::String>, int, v8::Local<v8::Value>*)’:
../node_modules/nan/nan.h:831:62: warning: ‘v8::Local<v8::Value> node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, v8::Local<v8::String>, int, v8::Local<v8::Value>*)’ is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations]
         v8::Isolate::GetCurrent(), target, symbol, argc, argv);
                                                              ^
In file included from ../node_modules/nan/nan.h:47:0,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/node.h:165:50: note: declared here
                 NODE_EXTERN v8::Local<v8::Value> MakeCallback(
                                                  ^
/home/matt/.node-gyp/10.0.0/include/node/node.h:88:42: note: in definition of macro ‘NODE_DEPRECATED’
     __attribute__((deprecated(message))) declarator
                                          ^~~~~~~~~~
In file included from ../src/posix.cc:1:0:
../node_modules/nan/nan.h: In function ‘v8::Local<v8::Value> Nan::MakeCallback(v8::Local<v8::Object>, const char*, int, v8::Local<v8::Value>*)’:
../node_modules/nan/nan.h:845:62: warning: ‘v8::Local<v8::Value> node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, const char*, int, v8::Local<v8::Value>*)’ is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations]
         v8::Isolate::GetCurrent(), target, method, argc, argv);
                                                              ^
In file included from ../node_modules/nan/nan.h:47:0,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/node.h:158:50: note: declared here
                 NODE_EXTERN v8::Local<v8::Value> MakeCallback(
                                                  ^
/home/matt/.node-gyp/10.0.0/include/node/node.h:88:42: note: in definition of macro ‘NODE_DEPRECATED’
     __attribute__((deprecated(message))) declarator
                                          ^~~~~~~~~~
In file included from ../src/posix.cc:1:0:
../node_modules/nan/nan.h: In member function ‘v8::Local<v8::Value> Nan::Callback::Call_(v8::Isolate*, v8::Local<v8::Object>, int, v8::Local<v8::Value>*) const’:
../node_modules/nan/nan.h:1453:5: warning: ‘v8::Local<v8::Value> node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*)’ is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations]
     ));
     ^
In file included from ../node_modules/nan/nan.h:47:0,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/node.h:172:50: note: declared here
                 NODE_EXTERN v8::Local<v8::Value> MakeCallback(
                                                  ^
/home/matt/.node-gyp/10.0.0/include/node/node.h:88:42: note: in definition of macro ‘NODE_DEPRECATED’
     __attribute__((deprecated(message))) declarator
                                          ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_chroot(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:118:51: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
     String::Utf8Value dir_path(info[0]->ToString());
                                                   ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_getrlimit(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:174:54: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
     String::Utf8Value rlimit_name(info[0]->ToString());
                                                      ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_setrlimit(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:214:54: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
     String::Utf8Value rlimit_name(info[0]->ToString());
                                                      ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_getpwnam(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:285:52: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
         String::Utf8Value pwnam(info[0]->ToString());
                                                    ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_getgrnam(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:326:52: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
         String::Utf8Value pwnam(info[0]->ToString());
                                                    ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_initgroups(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:366:47: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
     String::Utf8Value unam(info[0]->ToString());
                                               ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_openlog(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:442:48: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
     String::Utf8Value ident(info[0]->ToString());
                                                ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_syslog(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:474:50: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
     String::Utf8Value message(info[1]->ToString());
                                                  ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_sethostname(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:583:34: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
     String::Utf8Value str(info[0]);
                                  ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_swapon(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:609:34: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
     String::Utf8Value str(info[0]);
                                  ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
../src/posix.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE node_swapoff(Nan::NAN_METHOD_ARGS_TYPE)’:
../src/posix.cc:630:34: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
     String::Utf8Value str(info[0]);
                                  ^
In file included from /home/matt/.node-gyp/10.0.0/include/node/v8.h:26:0,
                 from /home/matt/.node-gyp/10.0.0/include/node/node.h:63,
                 from ../node_modules/nan/nan.h:47,
                 from ../src/posix.cc:1:
/home/matt/.node-gyp/10.0.0/include/node/v8.h:2822:28: note: declared here
                   explicit Utf8Value(Local<v8::Value> obj));
                            ^
/home/matt/.node-gyp/10.0.0/include/node/v8config.h:318:3: note: in definition of macro ‘V8_DEPRECATED’
   declarator __attribute__((deprecated(message)))
   ^~~~~~~~~~
posix.target.mk:95: recipe for target 'Release/obj.target/posix/src/posix.o' failed
make: *** [Release/obj.target/posix/src/posix.o] Error 1
make: Leaving directory '/home/matt/dev/bedrock-dev/bedrock-passport/test/node_modules/posix/build'
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/home/matt/.nvm/versions/node/v10.0.0/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:258:23)
gyp ERR! stack     at ChildProcess.emit (events.js:182:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:225:12)
gyp ERR! System Linux 4.9.0-6-amd64
gyp ERR! command "/home/matt/.nvm/versions/node/v10.0.0/bin/node" "/home/matt/.nvm/versions/node/v10.0.0/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /home/matt/dev/bedrock-dev/bedrock-passport/test/node_modules/posix
gyp ERR! node -v v10.0.0
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok

Consider updating Winston logger

Winston@1 is getting long in the tooth, the project is at v3

https://github.com/winstonjs/winston/blob/master/CHANGELOG.md

Address graceful handling of errors in bedrock events on startup

There are multiple open issues that are directly or indirectly related to this topic. Here's some code that demonstrates the problem so we can talk about what we would like to happen in these cases and where the error handling should occur.

The issue of the day for me is that we build MongoDB indexes inside of Bedrock event handlers. If an error occurs during indexing, the error is not properly surfaced and the server continues to operate when it should have failed to start IMO.

Here are my thoughts on what the solution looks like:

Top level applications should NOT be responsible for catching errors thrown by bedrock.start. We should continue to be able to simply do bedrock.start(); in top level applications as we are already doing now.
Internally, bedrock should:
- Properly log the error using the logging system.
- Shutdown as gracefully as possible. process.exit(1); would be fine for an immediate solution. In the fullness of time, Initiate whatever bedrock.exit process may be created.

const bedrock = require('bedrock');
const delay = require('delay');

bedrock.events.on('bedrock.init', async () => {
  (async () => {
    while (true) {
      console.log('BEDROCK IS STILL RUNNING');
      await delay(1000);
    }
  })();
});

bedrock.events.on('bedrock.ready', async () => {
  throw new Error('ERROR HERE');
});

// without this catch the error in the event is an uncaught promise rejection
// a catch is typically not included in top level applications

// if the catch handler does not shut down the server, the server will
// continue running in a broken state resulting in undefined behavior
bedrock.start().catch(e => console.log('LOG EVENT ERROR', e));

Related issues:
#69
#60
#56
#44

Move mocha out of core bedrock module

The mocha test infrastructure should move into a separate bedrock-mocha module.

See this comment: #15 (comment)

Security vulnerability in winston mail

If we can eliminate this, we will have a clean npm audit in some top-level applications.

┌──────────────────────────────────────────────────────────────────────────────┐
│                                Manual Review                                 │
│            Some vulnerabilities require your attention to resolve            │
│                                                                              │
│         Visit https://go.npm.me/audit-guide for additional guidance          │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low           │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ moment                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=2.19.3                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ bedrock                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ bedrock > winston-mail > emailjs > moment                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/532                             │
└───────────────┴──────────────────────────────────────────────────────────────┘

Master channel closed on exit

When the master process is exiting, we attempt to notify workers of this fact but the master channel has already closed. In node 4+, these messages will never be sent and an uncaught error is thrown as a result. We should remove this exit listener as it now serves no purpose. Instead, we should only send these messages prior to the master exiting. For example, when a worker suicides, the master process will exit, but it should wait until it has sent these messages to any workers that are still connected.

Also, while we're fixing this bug, note that worker.suicide has been deprecated and replaced with worker.exitedAfterDisconnect. See: https://nodejs.org/api/cluster.html#cluster_worker_suicide

Update `runOnceAsync` to match the behavior or `runOnce`

The current implementation of runOnceAsync requires a developer to create extra logic to ensure that the job ran once and other workers wait for its completion. This is not desired. The function should handle this logic internally to match the behavior as runOnce, namely all workers should wait until the job is done. Additionally, tests should be added to highlight this behavior.

change bedrock.config.jsonld.strictSSL default to true

bedrock.config.jsonld.strictSSL default in ./lib/config.js should be changed to a more secure true. This may break any dependents expecting the current false value.

Use rfdc module as bedrock.util.clone implementation

Replace lodash.clone with https://www.npmjs.com/package/rfdc

https://www.npmjs.com/package/rfdc#benchmarks

We will get a free performance bump wherever bedrock.util.clone is used.

Add option to handle errors from start().

3.x is returning a promise from start() that can result in unhandled exception errors unless the promise is accounted for. It seems like this API is fine, as it allows users to handle errors as appropriate in their app, but does result in a bit more code. A possible solution would be to add a config option that would control catching internal errors, and perhaps console.error them.

Audit/fix use of `logger.critical`

logger.critical is a custom error level and if an uncaught error lands here before we configure the logger (making critical a valid log level) an error is thrown that logger.critical is not a function.

https://github.com/digitalbazaar/bedrock/blob/master/lib/bedrock.js#L365

This is my theory anyway. This was detected when an address in use error was thrown on startup.

Need a general database API generation module

We have a number of different objects that we keep generating essentially the same database API boilerplate for each time a new object is invented. This is a problem because it's inefficient and error prone. We should come up with a solution to this problem.

Note that this problem or some variant of it is very traditional database/API problem where the solutions have ranged from introducing a different set of difficult problems to solve or causing even more headaches at worst. For example, we do not want to introduce or use an ORM system. The problems with these are myriad -- first and foremost that eliminate any semblance of fine grained control and hide the levers to work with your data in undesirable ways.

We could try and unify our database storage and create an abstraction layer that ends up treating everything as a giant graph (or dataset of graphs) of information, but this is also difficult to manage.

Perhaps the simplest thing we can do at this time is create a module that will auto-generate the basic APIs (and permissions, etc.) we want and allow overrides in individual modules as needed.

We have similar data access problems on frontends as well -- but we may have more flexibility with finding a solution there (it's unclear at this time).

cc: @msporny, @mattcollier, @davidlehn

Prevent `bedrock.start` from being called more than once.

Bad things happen if bedrock.start is called more than once. We should prevent this from happening by throwing an error.

Require config.paths to be set at the application level.

It's difficult to guess reasonable defaults for the config.paths values. Computed config defaults could be used that throw an error if the values are unset. This would force applications to set the values to something sane for their environment.

Add documentation about using bedrock-test with bedrock@2

Need to explain that bedrock does provide a test framework and links to bedrock-test.

Remove warnings related to setting config paths, etc.

See FIXMEs in lib/bedrock.js.

Add `initContext` helper

Now that a browser/node compatible structure for context modules has been found, it would be good to have a robust helper to initialize context modules.

The implementation here can be a starting place: digitalbazaar/bedrock-security-context#2

Here's some ideas from @davidlehn in response to the bedrock-security-context PR

It's going to be repeated in any bedrock-*-context package.
If for some reason this package wasn't a peer dep everywhere, and two packages with different versions were used, this may cause incorrect initialization. Putting in checks for such issues would be better in a bedrock common API call.
This assumes a one to one mapping of every constants entry with contexts entries. Two loops could address that.

Investigate need to use promisify with bedrock.events.emit

digitalbazaar/bedrock-express#9 switched from async lib to async/await. The bedrock.events.emit call had to be wrapped in promisify for it to function. Investigate if or why that is needed.

Fix SIGTERM handling

This branch demonstrates the issue:

https://github.com/digitalbazaar/bedrock/compare/create-sigterm

Our deployment infrastructures sends a SIGTERM when it wants to shut down applications. I was not seeing proper shutdown logging so I started investigating.

What we have now seems to work nicely with SIGINT, but SIGTERM is not hitting the event handlers we have for the purpose. I show that a handler outside the _runWorker function does see and SIGTERM event.

Symlink this branch to the app of your choice and use htop or similar to send SIGTERM to the bedrock worker.

Proposal for breaking change in jsonld document loader

It is currently considered best practice not to have jsonld document loaders fetch things from the Internet. The default document loader in Bedrock is designed to fetch things from the Internet.

https://github.com/digitalbazaar/bedrock/blob/master/lib/jsonld.js#L20
https://github.com/digitalbazaar/jsonld.js/blob/master/lib/documentLoaders/node.js

It has also been discussed that jsonld should not be baked into the base Bedrock module and should be in a module like bedrock-jsonld.

In order to make Bedrock secure by default, I propose a breaking change that implements a secure document loader.

We could at the same remove bedrock.jsonld all together and require that a new bedrock-jsonld module be installed and used instead.

How should we approach this issue?

Logging `BedrockError.cause` needs to be improved

bedrock/lib/util.js

Lines 80 to 82 in eaac0b9

    
           if(err.cause && !_isErrorPublic(err.cause)) { 
        
             object.cause = null; 
        
           }

This code assumes that every cause would be a BedrockError that would be explicitly marked as public.

It is not common practice to wrap every error that might be included in a BedrockError in another BedrockError.

I think the practical solution here is to say that the cause should be logged if the outer BedrockError is marked as public.

This of course could mean that a cause leaks data that should not be exposed in logs. However, the current state of affairs makes it impossible to debug applications issues in deployments.

Add test that writes to string not a file

Errors thrown in async event handers are not logged in bedrock logs

If any of the handlers for a bedrock.* event throw an error, an UnhandledPromiseRejectionWarning error is logged to the console, but is not logged in the bedrock logs. This makes diagnosing problems on deployments where the server is running as a service difficult.

Sample error: https://gist.github.com/mattcollier/07cdc9b07cb6b5e75d72c94cb6c612fa

It would be good to capture these errors and put them in the logs.

Node 10.x and async.waterfall

Due to this commit that landed in node 10.x: nodejs/node@0a0fbd5

The fs.close operation here: https://github.com/digitalbazaar/bedrock/blob/master/lib/loggers.js#L219

is calling it's callback with (err, undefined) instead of just (err) which results in async.waterfall passing undefined as the argument to the subsequent operation which in this case is expecting only callback where callback === undefined which leads to callback is not a function

I am working on a patch for node which may just be to revert to the previous behavior in node 8.x here: https://github.com/nodejs/node/blob/v8.x/lib/fs.js#L134-L136

BedrockError.toObject() should consider `showStack` config setting.

config.core.errors.showStack = false;

bedrock/lib/util.js

Lines 143 to 152 in 9a46769

    
           } else { 
        
             return { 
        
               message: err.message, 
        
               type: err.name, 
        
               details: { 
        
                 inspect: util.inspect(err, false, 10), 
        
                 stack: _parseStack(err.stack) 
        
               }, 
        
               cause: null 
        
             };

Note that inspect: util.inspect(...) as implemented here also includes a stack trace, then it is included in a different format in the stack field.

Neither of these takes into consideration the showStack config setting. This can lead to overly verbose errors, with two stack traces, being logged without any ability to change the behavior via the config.

TODO/FIXME Count 15

Run file logger setup in parallel

See https://github.com/digitalbazaar/bedrock/blob/main/lib/loggers/fileLogger.js

`runOnce` and `runOnceAsync` doesn't handle circular JSON

Messages sent (particularly errors thrown with express request objects that have cycles) are problematic -- and they need to be flattened before sent via IPC.

Mark file logger transport as deprecated; to be removed in next major release

Modern applications do not use file loggers, they send console logs through docker / cloud style tools.

Update README to show Vue examples or to instead just link to bedrock-vue / bedrock-angular

Implement clean shutdown code via bedrock.exit()

We need to implement cleaner shutdown code via bedrock.exit(). When bedrock.exit() is called, a bedrock.exit event should be emitted that is awaited on all workers -- and new workers should be prevented from starting up. Once all workers have exited, we can call cluster kill. A flag can be provided to bedrock.exit() with an optional timeout that will forcibly call cluster kill after the timeout.

node 4.x worker exit failure

With node 4.2+ (likely earlier 4.x too) all bedrock processes have an exiting problem.

api.exit() called.
Worker disconnect() called.
Cluster exit event handler gets event for same worker id but worker.suicide is false. According to https://nodejs.org/api/cluster.html it seems like it should be true.
Process prints critical error saying it got code 0.
Process exists with code 1.

Add parse error handling / use better stack parsing library

See lib/util.js _parseStack.

Separate test.js and test framework into bedrock-test

Since we're eyeing up a breaking release of bedrock, I think it would be good to split out the test framework, moving it into bedrock-test or some other module?

One issue that would be good to address is that the bedrock.started event is never makes it to top level apps when running test suites because the test suite is the first listener on on that event:

bedrock/lib/test.js

Lines 66 to 67 in 5a46f28

    
           events.on('bedrock.started', function() { 
        
             if(config.cli.command.name() === 'test') {

This is problematic when attempting to do testing in the ledger stack in particular which makes use of the bedrock.started event to initialize ledgers.

If the listener for running the test suite is registered last, when we require bedrock-test, then I think we would be better off.

LDP Support?

initial Notes:
https://www.w3.org/TR/ldp/
https://www.w3.org/2012/ldp/wiki/Main_Page
https://www.w3.org/2012/ldp/wiki/LDPNext2015_Charter

Uncaught error during `optimize` with worker set to zero

After optimization with bedrock-webpack, and with workers set to 0 in the config, there seems to be some issue shutting down child processes:

Node v6.11.3

2017-09-21T14:02:23.780Z - critical: master: uncaught error: Error: channel closed
    at ChildProcess.target.send (internal/child_process.js:584:16)
    at Worker.send (cluster.js:64:28)
    at process.<anonymous> (/home/bedrock/veres-delta/node_modules/bedrock/lib/bedrock.js:409:27)
    at emitOne (events.js:96:13)
    at process.emit (events.js:188:7)
    at process.exit (internal/process.js:164:15)
    at EventEmitter.<anonymous> (/home/bedrock/veres-delta/node_modules/bedrock/lib/bedrock.js:420:15)
    at emitThree (events.js:116:13)
    at EventEmitter.emit (events.js:194:7)
    at ChildProcess.<anonymous> (cluster.js:378:15)
    at ChildProcess.g (events.js:292:16)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:219:12)

Add test that adds logger early

Add minimum workers option.

An option could be added so that an application can enforce a minimum number of workers.
If an application wants at least 2 workers then the current method to scale by setting config.core.workers to 0 would not work on a 1 core host. A minWorkers could fix that. Alternatively, an application could just set this manually like config.core.workers = Math.max(os.cpus().length, 2).

Naming fix: remove use of `master` from APIs in next major release

Expose parsed command via an exported symbol instead of the config system

The next major release, remove config.cli.command and export it as a symbol instead.

Consider removal of `winston-mail` dependency

The mail transport in the Bedrock logging framework has not seen usage in recent years.

The winston-mail module has not had a release in approximately 3 years. The maintainers are not keeping up with dependency updates.

https://github.com/wavded/winston-mail/commits/master

Specifically, the package is still using emailjs@2 while emailjs@4 is the current release:

https://github.com/eleith/emailjs/commits/main

The emailjs@2 dependency graph includes a version of the ramda module which has a ReDoS vulnerability:
https://security.snyk.io/vuln/SNYK-JS-RAMDA-1582370

I propose that the mail transport be removed along with the winston-mail dependency.

`_runWorker` should catch rejected promises and exit

Use child logger instead of manually prefixing logger messages

See FIXMEs for child logger use or const logPrefix = ; usage.

Refactor package design to allow easier updates.

While adding uuid and delay helpers to core bedrock, the issue of maintenance of bedrock came up.
#34
#34 (comment)

Currently it is very difficult to do any type of semver major updates to bedrock itself. Such a version bump could potentially require every package that depends on bedrock to be updated at once. This has become intractable due to the number of direct and indirect dependents. For many classes of changes this may just be benign version compatibility warnings, but if an actual API change were to be made, it would be difficult, maybe impossible, to just use that updated version from one package. The recent changes involve adding helpers to bedrock. While convenient they do add more complexity into versioning and managing the exported API.

We should explore alternate package design methods that may help mitigate this issue. I imagine there are tradeoffs between ease of use and maintainability issues.

Remove non-operational `ran` variable and parameter.

https://github.com/digitalbazaar/bedrock/blob/master/lib/bedrock.js#L218

This variable is declared, but never changed, and therefore the feature it was intended to provide is not and has not been working. Rather than fix the deprecated implementation, the ran variable should just be removed.

Watch lodash Per Method Packages

With the major release of bedrock 4 we have started to use lodash per method packages.

bedrock/package.json

Lines 33 to 36 in 3af349a

    
           "lodash.get": "^4.4.2", 
        
           "lodash.set": "^4.3.2", 
        
           "lodash.template": "^4.5.0", 
        
           "lodash.topath": "^4.5.2",

However the maintainer of lodash has already declared they will be removing these packages with the release of lodash 5:

https://lodash.com/per-method-packages

Per Method Packages
Lodash methods are available in standalone per method packages like lodash.mapvalues, lodash.pickby, etc. These packages contain only the code the method depends on.

However, use of these packages is discouraged and they will be removed in v5.

What removed means is questionable, but in this github issue one of the maintainers of lodash says:

lodash/lodash#3838

The individual module packages won't be carried over to Lodash v5. I can't deprecate them with the npm deprecate method since that sends a log to stderr and causes some build scripts to fail. So I will simply let it fade away without a v5 update.

What this means is if a security vulnerability is found in any of the lodash method packages we are using there will be no support from the maintainer after lodash 5 is released. Removed could possibly mean the packages will be removed from npm entirely in the future however it appears removed just means they will not be part of lodash 5.

As a reminder the current version of lodash is 4 and the per method packages will be removed after the release of 5.
I could not find a roadmap for lodash 5's release. Version 4 was released July 10 2019.

Use new `node:` URLs where applicable?

This feature landed in Node 14.13.1

[EDIT] in 14.180+ this syntax can also be used with require().

https://nodejs.org/api/esm.html#node-imports

https://2ality.com/2021/12/node-protocol-imports.html
nodejs/node#38343
nodejs/node#37246
nodejs/node#35387

Among other things, this is in support of "a more extensive standard library in JavaScript" that will eventually be put into the node: namespace and allows the re-use of existing NPM package names without conflicts.
https://github.com/tc39/proposal-built-in-modules

Wait for logs to be written and output final message on bedrock.exit

https://github.com/winstonjs/winston#awaiting-logs-to-be-written-in-winston

https://nodejs.org/docs/latest-v14.x/api/stream.html#stream_event_finish

I guess the suggestion is that in the bedrock exit handler. We would register an even listener for logger.on('finish'), and then log "something" from within the exit handler itself followed by a logger.end(), and then when we see the finish event fire after that, we know the stream is done.

Originally posted by @mattcollier in #77 (comment)

Bedrock is not properly reporting exit code 1 on test failure

https://github.com/digitalbazaar/bedrock/compare/evergreen-tests?expand=1

Successful CI action reported there, but there was indeed a test failure shown here:

https://github.com/digitalbazaar/bedrock/runs/4451334012?check_suite_focus=true

Inspect the 'Run test with Node.js 14.x' section.

This was evidently broken by the recent signal handlers patch released in v4.4.2:
https://github.com/digitalbazaar/bedrock/blob/main/CHANGELOG.md#442---2021-11-04

If the bad test is applied to v4.4.1 return code of 1 is properly returned and CI properly reflects a failure:
https://github.com/digitalbazaar/bedrock/compare/4.4.1-bad-test?expand=1

The improper 0 return code can also be seen by doing. A 1 is expected when the test suite fails.

npm test
echo $?

	if(err.cause && !_isErrorPublic(err.cause)) {
	object.cause = null;
	}

	} else {
	return {
	message: err.message,
	type: err.name,
	details: {
	inspect: util.inspect(err, false, 10),
	stack: _parseStack(err.stack)
	},
	cause: null
	};

	events.on('bedrock.started', function() {
	if(config.cli.command.name() === 'test') {

	"lodash.get": "^4.4.2",
	"lodash.set": "^4.3.2",
	"lodash.template": "^4.5.0",
	"lodash.topath": "^4.5.2",

digitalbazaar / bedrock Goto Github PK

bedrock's People

Stargazers

Watchers

Forkers

bedrock's Issues

Recommend Projects

Recommend Topics

Recommend Org