[fix #10157] Make sure onLogin callbacks are called during startup

[brucejo75]

This repairs a timing issue. #10157

Essentially, the user is logged in before any onLogin callbacks can be set up. To remedy the problem:

I added a new internal function.

AccountsCommon#_startupCallback internal function

• _startupCallback is called is called right after an onLogin callback is registered.
• On the client _startupCallback checks if the user is logged in (non reactively), if true then the newly registered callback is scheduled to execute at the back of the event loop. This makes sure that all necessaryMeteor.startup callbacks complete before the onLogin callback executes if the onLogin is called within a Meteor.startup.
• On the server the function is a noop.

[brucejo75]

@benjamn , I was just checking where my changes were in the Meteor repo and I could not find them.

Should I have built this PR off the 1.8.2 branch originally?

I noticed it was still not merged in and that I was required to push my changes back to my repo, just did it.

[KoenLav]

@brucejo75 we've experienced the same issue and went around it by doing the following:

if (Meteor.user()) {
callback();
}

Accounts.onLogin(callback);

I think your approach is similar, but why not just "fix" the onLogin callback (I can't imagine a scenario where someone would not want to have that callback executed when the user is, by chance, logged in before AccountsCommon is constructed.

[brucejo75]

Hi @KoenLav,

I am trying to parse your issue:

1. onLogin is a common function
2. A user being logged in prior to declaring an onLogin callback I had assumed was uniquely a client issue. And this is the only place I had seen it.

Are you saying that I should not distinguish between client or server and simply make the callback if the user is logged in (client or server)?

On the client side there is an explicit login during AccountsClient instantiation (if _autoLoginEnabled then there is a loginWithToken). I considered trying to delay this login to occur after I could be sure all onLogin functions had been declared. But I thought I might be introducing unexpected and unacceptable behavior.

On the server side I do not see anything like that. Plus the Meteor.userId() & Meteor.user() functions have very limited scope on the server. So I decided to not perturb the server side by implementing a noop.

Given this information I thought it was prudent to only implement the client side.

I have not seen a bug where this is an issue. Is it something you have encountered?

[brucejo75]

@benjamn, will this make the cut for 1.8.2?

[KoenLav]

@brucejo75 I don't think this issue occurs on the server either, maybe I didn't explain properly.

What I meant with fixing the login callback would be: if the user is already logged in when adding a loggedIn callback, it should probably fire right away (currently this doesn't happen, which, in my opinion, is the issue).

[brucejo75]

@KoenLav ,

That is why I was confused because that is exactly what my changes do. This function is called right after the callback is registered:

  // _startupCallback executes on onLogin callbacks
  // at registration time if already logged in
  // this can happen when new AccountsClient is created
  // before callbacks are registered see #10157
  _startupCallback(callbackId) {
    // Are we already logged in?
    if(this.connection._userId) {
      // then go ahead and call the callback that we just registered
      let cb = this._onLoginHook.callbacks[callbackId];
      // if already logged in before handler is registered
      // safe to assume that type is a 'resume'
      // execute the callback at the end of the queue so that
      // Meteor.startup can complete before any embedded onLogin
      // callbacks would execute
      Meteor.setTimeout(() => cb({loginDetails: {type: 'resume'}}), 0);
    }
  }

When I make the callback (via a setTimeout) I am deferring the callback until after all Meteor.startup code has completed. That is a convenience for the user if they reference initialized code/variables in their onLogin (which was exactly my case).

I might make the case that all callback should defer until all Meteor.startup's complete.

[KoenLav]

Apologies, I thought you added a new method which produced the expected behavior, rather than improved the existing onLogin method.

[benjamn]

Sorry I haven't had a chance to follow up on this properly. For what it's worth, this is definitely on the list of changes we plan to ship with Meteor 1.8.2.

I think I want to incorporate this functionality into the Hook class, so there's a clear moment when the hook has been fired, and any callbacks added after that will be fired immediately (or in the next tick of the event loop). That's essentially what you're doing, of course, but it currently requires manual effort in the accounts-base package.

If that makes sense and you have time, feel free to give it a go. Otherwise I can make those changes after #10585 is finished and merged into release-1.8.2 (this PR isn't related to that PR; I'm just a little preoccupied with #10585 right now).

[brucejo75]

@benjamn, I will not get a chance to look at it closely for 2 weeks, deadline and out of office. I quickly glanced at it:

• I can see easily how to enhance the Hook class to get the same effect.
• I think it would require scheduling at the next tick because of the returned stop() function that may be referenced in the callback.
• I think it may be a fair amount of work to check all the other Hook instances to make sure it does not create unwanted behavior for those other instances... (maybe the tests cover it well)

[brucejo75]

@benjamn, I took a closer look at the Hook class. I need some guidance on how you want to implement this.

1. When a callback is registered, the Hook class simply stores the callback in an array of callbacks.
2. When the callbacks are executed in the each method, the Hook class hands the callback back to the caller of the each method. That way the caller can add the appropriate arguments to the callback function.

When registering the callback, I can detect that the each method has already run so I know that the callback should be immediately called. But I do not know what arguments to use for the callback.

Proposal

Add an optional argument to the register method. If the argument is set & the callbacks have already fired then in the register method make the callback immediately with the passed argument.

• Hides the internals of the Hook class and provides the required feature.
• The caller needs to be explicit about taking advantage of this feature, maybe a benefit for the Hook class clients other than accounts.
• Unfortunately, to take advantage of it the caller needs to be modified. For the accounts package, it would be the addition of the argument {type: 'resume'} here.

Let me know if that is what you were thinking. I can get a PR pulled together.

[brucejo75]

Hi @benjamn, any thoughts on my proposal above?

[brucejo75]

Hi @benjamn

I was thinking about this some more and thought that I might be able to save the iterator from the each call and then use it as the callback on a register.

I do not think this would work, because the Hook class is defined pretty liberally. And the caller could call each multiple times with any number of iterators, so I should not make any assumptions about the iterator that should be called.

Given this information, I think that handing arguments back to the register call is a bad idea too. The iterator could conceivably make more side effect calls. So ideally, an iterator needs to be handed to the register call as well.

But then to use this capability it in the example of the accounts package would require making the iterator available for reuse in the package. Now an implementation starts to look as messy as simply calling the onLogin callback as I did in the accounts package with this PR.

I am not sure that adding this capability to the Hook class is a good idea. And I think that what I did on the Accounts package was done with a fair amount of knowledge of how the Hook class was used.

Recommend taking this PR as an accounts package feature.

[CLAassistant]

All committers have signed the CLA.

[benjamn]

Finally getting this in—sorry for the long wait @brucejo75.