digitalbazaar / bedrock Goto Github PK
View Code? Open in Web Editor NEWBedrock: A core foundation for rich Web applications.
License: Apache License 2.0
Bedrock: A core foundation for rich Web applications.
License: Apache License 2.0
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
Winston@1 is getting long in the tooth, the project is at v3
https://github.com/winstonjs/winston/blob/master/CHANGELOG.md
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:
bedrock.start
. We should continue to be able to simply do bedrock.start();
in top level applications as we are already doing now.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));
The mocha test infrastructure should move into a separate bedrock-mocha
module.
See this comment: #15 (comment)
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 │
└───────────────┴──────────────────────────────────────────────────────────────┘
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
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.
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.
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.
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.
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.
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
Bad things happen if bedrock.start
is called more than once. We should prevent this from happening by throwing an error.
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.
Need to explain that bedrock does provide a test framework and links to bedrock-test.
See FIXMEs in lib/bedrock.js
.
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.
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.
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.
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?
Lines 80 to 82 in eaac0b9
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.
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.
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
config.core.errors.showStack = false;
Lines 143 to 152 in 9a46769
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: test w/custom logger that writes to string, not file
Commit: (ccfc29d) Setup test suite with bedrock-test@4.
File: tests/mocha/test.js:642
Matthew Collier commented 8 months ago
TODO: add test that adds logger early
Commit: (ccfc29d) Setup test suite with bedrock-test@4.
File: tests/mocha/test.js:636
Matthew Collier commented 8 months ago
FIXME: rework this error handling and callers
Commit: (257fcd7) Use wrapper for loggers.
File: lib/loggers.js:270
Dave Longley commented 5 years ago
TODO: run in parallel
Commit: (749029d) Convert remaining async lib use to promises.
File: lib/loggers.js:202
David I. Lehn commented 8 months ago
TODO: simplify with cluster.on('online')?
Commit: (37cb19c) Add bedrock-cli feature.
File: lib/bedrock.js:509
Dave Longley commented 5 years ago
FIXME: use child logger
Commit: (c2f318c) Use workaround for master/worker child logger.
File: lib/bedrock.js:452
David I. Lehn commented 7 months ago
TODO: does this need adjusting after fixing the worker cwd
issue?
Commit: (3fba9c5) Reorganize code into functions; fix bug with runOnce notifications.
File: lib/bedrock.js:377
Dave Longley commented 5 years ago
FIXME: use child logger
Commit: (c2f318c) Use workaround for master/worker child logger.
File: lib/bedrock.js:371
David I. Lehn commented 7 months ago
FIXME: v2.0.0: remove warning and default and throw exception .
Commit: (5f2efcb) Deprecated config.paths defaults.
File: lib/bedrock.js:56
David I. Lehn commented 3 years ago
FIXME: v2.0.0: remove warning and default and throw exception .
Commit: (5f2efcb) Deprecated config.paths defaults.
File: lib/bedrock.js:43
David I. Lehn commented 3 years ago
FIXME: v2.0.0: remove when removing warnings below.
Commit: (5f2efcb) Deprecated config.paths defaults.
File: lib/bedrock.js:36
David I. Lehn commented 3 years ago
Commit: (8e0950c) Comment out TODO.
File: README.md:715
Dave Longley commented 5 years ago
FIXME also check if a validation type?
Commit: (7d9c451) Initial commit of bedrock.
File: lib/bedrock/tools.js:110
Dave Longley commented 6 years ago
* TODO: look into better stack parsing libraries.
Commit: (4d3fafd) Parse stack errors.
File: lib/bedrock/tools.js:76
David I. Lehn commented 6 years ago
FIXME: add parse error handling
Commit: (4d3fafd) Parse stack errors.
File: lib/bedrock/tools.js:98
David I. Lehn commented 6 years ago
Messages sent (particularly errors thrown with express request objects that have cycles) are problematic -- and they need to be flattened before sent via IPC.
Modern applications do not use file loggers, they send console logs through docker / cloud style tools.
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.
With node 4.2+ (likely earlier 4.x too) all bedrock processes have an exiting problem.
api.exit()
called.disconnect()
called.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
.0
.1
.See lib/util.js
_parseStack
.
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:
Lines 66 to 67 in 5a46f28
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.
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)
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)
.
The next major release, remove config.cli.command
and export it as a symbol instead.
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.
See FIXMEs for child logger use or const logPrefix = ;
usage.
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.
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.
With the major release of bedrock 4
we have started to use lodash per method packages.
Lines 33 to 36 in 3af349a
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:
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.
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
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)
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 $?
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.