Meteor-feature-requests: Only wrap OAuth endpoint in a Fiber if necessary

[MKRazz]

As I understand it, WebApp.connectHandlers.use callbacks already run in a fiber, but the endpoint used by the OAuth package wraps itself in a fiber without checking.

WebApp.connectHandlers.use(function(req, res, next) {
  // Need to create a Fiber since we're using synchronous http calls and nothing
  // else is wrapping this in a fiber automatically
  Fiber(function () {
    middleware(req, res, next);
  }).run();
});

https://github.com/meteor/meteor/blob/b52c6587d7542c0f27481a3bee8c65be06068ac1/packages/oauth/oauth_server.js#L142-L148

In my project I'm attempting to wrap all connectHandlers callbacks to provide some context similar to DDP._CurrentMethodInvocation and DDP._CurrentPublicationInvocation but it's failing here because of the fiber call.

I copied the package locally and made this change and it fixed my issue, but if it's stable enough to get into core that'd be great so I don't need to maintain a local copy of oauth for this.

WebApp.connectHandlers.use(function(req, res, next) {
  var runInFiber = function() {
      middleware(req, res, next);
  };

  if (Fiber.current)
      return runInFiber();
  Fiber(runInFiber).run();
});

[mitar]

Maybe it is easier to just use Meteor.bindEnvironment? It does such check internally?

[MKRazz]

Even better.

[hwillson]

@MKRazz Given the suggestion in https://github.com/meteor/meteor-feature-requests/issues/156#issuecomment-317118939, do you still think this FR should be considered?

[mitar]

@hwillson, my comment was just that a PR should simply change blindly using Fiber into a Meteor.bindEnvironment.

[hwillson]

Perfect, thanks @mitar!

[hwillson]

I've re-read the original issue (now that I've finished my coffee and am a little more awake 😪), and reviewed the oauth source. Let's make this more defensive - 👍 for the Meteor.bindEnvironment change. Marking this as pull-requests-encouraged; @MKRazz are you interested in working on a PR?

[mitar]

And make a test for this as well, please. :-)

[MKRazz]

@hwillson I can make the PR but it probably won't be right away because I haven't made one for Meteor yet, so I probably need to go over the rules and sign the agreement and whatnot.

[mitar]

@hwillson, BTW, I think this would be an interesting next thing do to in Meteor core:

In my project I'm attempting to wrap all connectHandlers callbacks to provide some context similar to DDP._CurrentMethodInvocation and DDP._CurrentPublicationInvocation but it's failing here because of the fiber call.

Imagine if Meteor.userId() would work also inside connectHandlers. Maybe DDP._CurrentConnectionHandler?

cc @zimme

[zimme]

This sounds like a nice thing on the surface, need to check how hard it would be to create the context to be sure though.

[MKRazz]

Since connectHandlers are before page load / DDP, would the user id be available at that stage?

[MKRazz]

Upon closer inspection of Meteor.bindEnvironment, it looks like it won't be equivalent to my original code.

My original code will run the middleware function in a fiber if it is not already in one, or run it directly if it is. Meteor.bindEnvironment must be run in a fiber, so if I put it inside the callback and there's no fiber, it will throw instead of running it in a fiber. If I wrap the whole callback inside Meteor.bindEnvironment it will bind the callback to the environmental variables available at the moment of binding, which is probably none because that runs when the app starts up vs when the route is being hit.

So I think it'll be better to use my original version so that if the callback is wrapped in an environment it will be available within, and if it isn't wrapped in an environment it will continue to work as it does now.

[mitar]

Are you sure? Here it has if which makes it work outside Fiber as well: https://github.com/meteor/meteor/blob/master/packages/meteor/dynamics_nodejs.js#L122

[mitar]

But yes, you probably researched this more, so ignore my comments. :-)

[MKRazz]

@mitar No worries, I appreciate the input, big fan of peerlibrary btw :)

That's the part that had me too, the issue is this part: https://github.com/meteor/meteor/blob/78052a65016dba28086ee08dff9353233f448e34/packages/meteor/dynamics_nodejs.js#L85-L85

As I'm understanding it, the purpose of Meteor.bindEnvironment is to carry the current meteor environment with you into callbacks. So if you use a callback within a meteor method you can wrap it with bindEnvironment to restore the data in DDP._CurrentMethodInvocation.get() once the callback fires, so you can access the same environment you had when you bound within the method even if it changed in the interim. So it's not designed to bind without an existing environment.

Maybe it'd be cool if we had something like Meteor.ensureFiber(callback) that just does that fiber check.

[mitar]

Aha, it requires that fiber has to exist when you call it, but later on, when callback is called, it makes sure fiber, with same environment, is there, no matter what (if it is called from existing fiber or not).

So yes, we can then probably manually create a Fiber here.

Is it possible to access connection here? Maybe without userId set?

[MKRazz]

Yep!

I don't remember all of what is on connection, but it can be at least partially reproduced using the info from req, that's what I'm doing with it.

[MKRazz]

With this update, https://github.com/meteor/meteor/commit/3b18863, it always wraps connect handlers in a Fiber now, so the new solution here seems to be to skip Fiber handling entirely and just call the middleware directly.