Add per-request child context #188

bajtos · 2017-04-21T13:35:36Z

Create a new child context for each request handled by the app. Add the following two bindings to this new context:

http.request
http.response

This is a pre-requisite for #186 which I am implementing as part of the time allocated in https://github.com/strongloop-internal/scrum-asteroid/issues/117.

cc @bajtos @raymondfeng @ritch @superkhau

bajtos · 2017-04-21T13:36:22Z

Connect to strongloop-internal/scrum-asteroid#117

bajtos · 2017-04-21T13:37:01Z

packages/context/src/context.ts

    this.registry = new Map();
+    this.bind('ctx').to(this);


This is not ideal, see the next comment explaining why I have added this.

Maybe we should do this... (with some way of turning off the warning in the tests...

ctx.bind('ctx').toDynamicValue(() => { console.warn('Hey... don't use @inject('ctx') !!!'); return ctx; });

Not a bad idea.

bajtos · 2017-04-21T14:07:05Z

packages/loopback/test/acceptance/routing/routing.acceptance.ts

+
+    @api(spec)
+    class FlagController {
+      constructor(@inject('ctx') private ctx: Context) {


This is the test injecting the full context.

Here is the problem I am trying to solve: I need to write a test that fails when the same app context is used for all requests. To detect that, I need one of the following:

Some way how to modify a binding in the (per-request) context - that's what I am doing now

Use advanced async/timing control ensure that two requests run in parallel are executed in lockstep. Then I can inject the request object to the controller, write a method that returns the request url including the query string, and use a different query string in each request value. When the timing is right, the controller method executed for the first request will receive the second request from @inject, and the test fails. This is not possible at the moment, because I have no control over the async flow inside Router/Context/invoke. I think this could be achieved if context supported async bindings.

Can you think of any better way how to test per-request context instances?

I have a holistic proposal for this... I'd call this the Black Hole Problem - you can bind at the app level, and those bindings are inherited now (thanks to this PR), but you cannot rebind per request.

@bajtos and I will have some realtime discussion about this soon and link to the proposal here.

For the purpose of testing new instances, can you cache ctx to the class and assert the new one is not the same as the cached one?

That's basically what I am doing here. Instead of comparing ctx references (which yield ugly assertion failures), I bind a value to the per-request context and then verify that the second request does not see the value set by the first request.

The reason why I don't like this approach is that user code should never access the DI container (ctx) directly, that breaks the whole DependencyInjection/InversionOfControl design.

raymondfeng · 2017-04-21T16:03:48Z

packages/context/src/context.ts

@@ -11,8 +11,9 @@ export type Constructor<T> = new(...args) => T;
 export class Context {
  private registry: Map<string, Binding>;

-  constructor() {
+  constructor(private _parent?: Context) {
    this.registry = new Map();


Each instance of Context should have a unique id for tracing purposes.

Interesting idea. I think I could use that unique id in my test instead of @inject('ctx').

@raymondfeng How do you propose to generate that unique id? Should we just increase a number, or should we use uuid?

Do you have any other comments, or does the rest of the patch look good to you?

I think uuid is good enough. We should also check out http://opentracing.io/documentation/.

On the second thought, let's move the unique tracing id out of scope of this patch - see #209

raymondfeng · 2017-04-21T16:16:11Z

packages/loopback/lib/application.ts

-        // TODO(bajtos) Create a new nested/child per-request Context
-        const requestContext = this;
+        const requestContext = new Context(this);
+        requestContext.bind('http.request').to(req);


I wonder if we should use an even more qualified name as transport.http.request. Such namespace/package can help us organize the context. Food for thoughts.

I think we should qualify it when we actually implement other protocols.

raymondfeng

LGTM with some suggestions.

superkhau · 2017-04-21T20:54:16Z

packages/loopback/lib/application.ts

-        // TODO(bajtos) Create a new nested/child per-request Context
-        const requestContext = this;
+        const requestContext = new Context(this);
+        requestContext.bind('http.request').to(req);


I think we should qualify it when we actually implement other protocols.

bajtos · 2017-04-24T11:59:55Z

Let's land async context bindings via #193 before continuing the work on this patch.

bajtos · 2017-04-27T13:00:34Z

this.bind('ctx').to(this)

@bajtos
This is not ideal, see the next comment explaining why I have added this.

@ritch
Maybe we should do this... (with some way of turning off the warning in the tests...
ctx.bind('ctx').toDynamicValue(() => {
  console.warn('Hey... don\'t use @inject(\'ctx\') !!!');
  return ctx;
});
@raymondfeng
Not a bad idea.

Instead of creating a binding that should not be used, and adding even more code to allow tests to suppress the warning, I found a solution that I consider much more elegant: create this special context binding directly inside the test.

app.bind('context').getValue = ctx => ctx;

bajtos added this to the Sprint 34 - Asteroid milestone Apr 21, 2017

bajtos self-assigned this Apr 21, 2017

bajtos requested review from ritch, raymondfeng and superkhau April 21, 2017 13:35

bajtos added the review label Apr 21, 2017

bajtos commented Apr 21, 2017

View reviewed changes

bajtos mentioned this pull request Apr 21, 2017

Authentication: Invoking passport strategy from router #186

Closed

raymondfeng reviewed Apr 21, 2017

View reviewed changes

raymondfeng approved these changes Apr 21, 2017

View reviewed changes

superkhau approved these changes Apr 21, 2017

View reviewed changes

bajtos mentioned this pull request Apr 24, 2017

Add support for mixed sync/async bindings #193

Merged

bajtos mentioned this pull request Apr 25, 2017

context: Unique tracing id per context instance #209

Closed

superkhau approved these changes Apr 26, 2017

View reviewed changes

bajtos force-pushed the feature/per-request-context branch from f733e49 to bd4eb80 Compare April 27, 2017 12:26

Add per-request child context

14b48ec

bajtos force-pushed the feature/per-request-context branch from bd4eb80 to 14b48ec Compare April 27, 2017 13:00

bajtos merged commit a422dfb into master Apr 27, 2017

bajtos deleted the feature/per-request-context branch April 27, 2017 13:15

bajtos removed the review label Apr 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add per-request child context #188

Add per-request child context #188

bajtos commented Apr 21, 2017 •

edited

Loading

bajtos commented Apr 21, 2017

bajtos Apr 21, 2017

ritch Apr 21, 2017

raymondfeng Apr 21, 2017

bajtos Apr 21, 2017

ritch Apr 21, 2017

raymondfeng Apr 21, 2017

bajtos Apr 24, 2017

bajtos Apr 24, 2017

raymondfeng Apr 21, 2017

bajtos Apr 21, 2017

raymondfeng Apr 21, 2017

bajtos Apr 25, 2017

raymondfeng Apr 21, 2017

superkhau Apr 21, 2017

raymondfeng left a comment

superkhau Apr 21, 2017

bajtos commented Apr 24, 2017

bajtos commented Apr 27, 2017

Add per-request child context #188

Add per-request child context #188

Conversation

bajtos commented Apr 21, 2017 • edited Loading

bajtos commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Apr 24, 2017

bajtos commented Apr 27, 2017

bajtos commented Apr 21, 2017 •

edited

Loading