Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binding context to promise chain #498

Open
maxbrieiev opened this issue May 15, 2017 · 2 comments
Open

Binding context to promise chain #498

maxbrieiev opened this issue May 15, 2017 · 2 comments

Comments

@maxbrieiev
Copy link

Hi!

I want to bind some context to promise chain for the purpose of context aware logging. For this my idea is to extend promise API. Here is an example:

const when = require('when');

let getUserSession = when('get user session');
when()
  .bindToCtx({taskId: 123})
  .then(function () {
    return getUserSession.log('Got session');
  })
  .then(function (session) {
    return when('get some data from db')
      .log('Got data from DB')
      .then(function (data) {});
  })
  .log('task finished');

I would expect to see something like this in the log output:

debug:task:123: Got session 
debug:task:123: Got data from DB
debug:task:123: task finished

My idea was to extend Promise along following lines:

const when = require('when');

function contextLogging(Promise) {
  Promise.createContext = function(p, context) {};
  Promise.enterContext = function (p) {};
  Promise.exitContext = function () {};

  Promise.prototype.bindToCtx = function (ctx) {};
  Promise.prototype.log = function (msg) {};

  return Promise;
}

At first I thought that when/monitor/PromiseMonitor would be a good start. But after playing with it for some time, I am a bit confused. To me it appears that it doesn't track context properly. Here is an example:

require('when/monitor/console');
var when = require('when');

var p1 = when(1);
var p2 = when(2);

p1.then(function f1() {
  return p2
    .then(function f2() {
      return 3;
    })
    .then(function f3() {
      doh();
    });
});

Here I expect that p2 runs within the context of p1. However, the traceback doesn't give any insights on this:

% node test.js
[promises] Unhandled rejections: 1
ReferenceError: doh is not defined
    at f3 (/home/mbreev/test.js:13:7)
from execution context:
    at run (bootstrap_node.js:427:7)
from execution context:
    at _combinedTickCallback (internal/process/next_tick.js:73:7)
    at process._tickCallback (internal/process/next_tick.js:104:9)
from execution context:
    at Object.<anonymous> (/home/mbreev/test.js:5:10)

So my question is: would PromiseMonitor be a good example to implement context binding to chain of promises?

Is it feasible at all to extend Promise this way?

Thank you in advance for any suggestions.

@rkaw92
Copy link
Contributor

rkaw92 commented May 15, 2017

Hi,

First of all, never use when/monitor/console in production. In fall 2015, we had a very hard-to-debug issue including memory leaks on production (like #194 - for us it crashed apps and made the GC stall at 1.5GB), and that module turned out to be the reason. I am pretty sure the issue is still there.

Other than that, it seems to me that you are trying to use Promises as a context-aware task runner. It may not be a good fit, because of two reasons:

  1. It dilutes the concept of a Promise as a future value and adds some context-passing responsibility to it. This violates the SRP and thus warrants a separate class / unit to implement this feature. Moreover, readers of your code may be prone to think that your custom Promise class does flow control (can I redirect the flow of code using enterContext, exitContext?), which it hopefully does not.
  2. Logging will usually need a log target, which may be a console instance but also something like a syslog socket fd, etc. If you extend the Promise prototype, you are effectively imposing a singleton logger on the users of the library throughout the project. This is bad.

You should typically inject loggers into your logic. A handy library that can help you manage and enrich logger state is bunyan, which allows "hierarchical" loggers. One combination with when that is particularly useful is Promise#tap, which applies a side effect without modifying the resolution value.

@maxbrieiev
Copy link
Author

Thank you for your comment.

Maybe extending Promise prototype with new methods is bad idea.

My initial thoughts were as following. The chain of promises constitutes some asynchronous 'task'. Promises wait on each other as long as they are returned from functions. When the last promise is resolved the task finishes. Why not use this promise machinery to attach some context, so the group of related asynchronous operations could be treated together. The main purpose here is context aware logging. It would be useful to have some hooks provided by the library.

What I want to have is something like node domains feature ( https://nodejs.org/api/domain.html ). The API doesn't have to be the extension of Promise. It can be for example, something like this:

var promise = db.cal('sql');
var log = getLoggingFromExecutionContext(promise);
log.debug('fetching data from db');

I have previously looked at bunyan, but I didn't find a way to group related asynchronous operations together. I will take a look once more at hierarchical logging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants