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

Session hooks #1611

Closed
wants to merge 23 commits into
base: devel
from

Conversation

Projects
None yet
6 participants
@awwx
Contributor

awwx commented Nov 15, 2013

Add Meteor.server.onConnection API.

Move handling of login tokens from livedata to accounts.

@estark37

This comment has been minimized.

estark37 commented on packages/livedata/livedata_server.js in b4de98a Nov 14, 2013

Hmm... I think so, for the same reason that we defer _deactivateAllSubscriptions. At some point in the very near future we'll have code in accounts-base that closes a bunch of connections in a loop, and we don't want to wait on all the close callbacks for one connection to return before closing the other connections.

@estark37

This comment has been minimized.

estark37 commented on packages/livedata/livedata_server.js in b4de98a Nov 14, 2013

I think this should perhaps be underscored to match _pendingReady, _isSending, etc., since we're only using it from within the Session class.

@estark37

This comment has been minimized.

I suspect this is because the computation rerunning is triggered by the close event firing on the stream's connection object (

if (self.currentConnection !== this)
), and the event listeners on the connection object don't run in fibers. Not sure what if anything we should do about this, except maybe update the comment to explain why the bindEnvironment is there. Let me know if that explanation doesn't make sense or seems wrong.

This comment has been minimized.

Owner

awwx replied Nov 14, 2013

That makes sense. But, the unit test code at this point is using all public API's, isn't it? A developer could be checking the connection status in the same way. So a developer shouldn't need to use a bindEnvironment wrapper here, that should happen somehow in the lower level code so it gets taken care of for them, yes?

awwx added some commits Nov 15, 2013

Wrap calling session close callback in Meteor.defer, so that a bunch
of connections can be closed without waiting for the close callbacks
on one connection to return before closing the other connections.

Underscore internal Session field `_closeCallbacks`.

Update comment to explain the cause of the problem requiring the use
of `Meteor.bindEnvironment` with Meteor's public API.
// sessionId -> SessionHandle
var sessionHandles = {};

This comment has been minimized.

@awwx

awwx Nov 15, 2013

Contributor

Note an alternative is if Meteor.server had an API to get the session from a session id, accounts wouldn't need to maintain its own mapping here.

@estark37

This comment has been minimized.

Hrm, I think the cloning might actually be unnecessary... The onClose callbacks are deferred and we don't yield in this loop, so I think sessionsByLoginToken won't be modified until we're done looping.

On the other hand, cloning probably doesn't hurt anything, and it's probably a good safety belt. Maybe you could just update the comment to say that it's technically not necessary to do the cloning but better not to rely on livedata deferring the callbacks. (Does that sound like a true statement? I'm not sure I've completely convinced myself...)

This comment has been minimized.

Owner

awwx replied Nov 15, 2013

Right, this loop was written before we added Meteor.defer to calling the onClose callbacks.

It's probably best not to clone if we don't need to... why clone here, and not in a whole bunch of other places where we don't need to clone, but we could just in case something changed in the future?

@estark37

This comment has been minimized.

I know you didn't write the map originally, but would you mind changing the map over userTokens to _.pluck(userTokens, "token")?

This comment has been minimized.

Owner

awwx replied Nov 15, 2013

ok

@estark37

This comment has been minimized.

Maybe getLoginToken and setLoginToken should be underscored? (Consistent with Accounts._generateStampedLoginToken which is used by other accounts packages like these ones.) I don't think we'll want to document these or expose them to users.

This comment has been minimized.

Owner

awwx replied Nov 15, 2013

ok

Underscore internal `_getLoginToken` and `_setLoginToken`.
`closeSessionsForTokens` doesn't need to clone `sessionsByLoginToken`
because `onClose` callbacks are deferred.

Simplify `closeTokensForUser` by using `_.pluck`.
@@ -1,3 +1,70 @@
// token -> list of session ids

This comment has been minimized.

@n1mmy

n1mmy Nov 19, 2013

Member

Code organization for easy reading: this should go in the "RECONNECT TOKENS" or "TOKEN EXPIRATION" section, not at the top of the file.

self.sessionHandle = {
id: self.id,
close: function () {
self.server._destroySession(self);

This comment has been minimized.

@n1mmy

n1mmy Nov 19, 2013

Member

Hrm, I'm not sure _destroySession is quite right. I think it is intended to be called once per session... I don't think it is particularly idempotent.

One way it isn't idempotent is that the onClose callbacks can be called more than once. Eg:

  Meteor.server.onConnection(function (sessionHandle) {
    console.log("GOT SH", sessionHandle.id);
    sessionHandle.onClose(function () {
      console.log("CLOSE SH", sessionHandle.id);
    });
    var close = function () {
      sessionHandle.close();
    };
    Meteor.setTimeout(close, 1000);
    Meteor.setTimeout(close, 1500);
  });

results in CLOSE SH being printed twice for each session handle. The onClose function should fire exactly once.

There may also be other ways it isn't idempotent.

// treats it as semi-deactivated (it will ignore incoming callbacks, etc).
self._deactivateAllSubscriptions();
});
// Drop the merge box data immediately.
self.collectionViews = {};
self.inQueue = null;
Meteor.defer(function () {

This comment has been minimized.

@n1mmy

n1mmy Nov 19, 2013

Member

Comment or line spacing or something. This all blends together.

}
var invocation = new MethodInvocation({
isSimulation: false,
userId: userId,
setUserId: setUserId,
_setLoginToken: setLoginToken,
sessionId: sessionId,
// XXX the Server object doesn't have a `sessionData` field.

This comment has been minimized.

@n1mmy

n1mmy Nov 19, 2013

Member

Er? What does this comment mean?

This comment has been minimized.

@awwx

awwx Nov 19, 2013

Contributor

I noticed that when a method calls another method on the server, the inner method is supplied sessionData from the Server object (which doesn't have the sessionData field), so I marked this with an "XXX" comment.

This comment has been minimized.

@n1mmy

n1mmy Nov 20, 2013

Member

Ah, yeah. Oops. Should probably fix this by setting to either currentInvocation.sessionData or null if no currentInvocation.

var callbackHandle = Meteor.server.onConnection(function (sessionHandle) {
callbackHandle.stop();
// Wait for connection to be closed on the client side.
Deps.autorun(function (computation) {

This comment has been minimized.

@n1mmy

n1mmy Nov 19, 2013

Member

[This is already on your list, adding mostly as a note to my future self.] Deps.autorun on the server is no good. This should be removed.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Nov 19, 2013

This is looking good! Some comments inline.

Also, can you please add some docs for the new public APIs? I think users will be really excited to use them!

awwx added some commits Nov 19, 2013

Run nodejs stream client callbacks in a fiber.
Poll instead of using Deps.autorun in server test.

When polling the client connection, tests don't have a chance to
disconnect before the stream client automatically reconnects, so add
an option to disable retries for testing.

Callers of `Meteor.bindEnvironment` often have the `onException`
argument print the exception stack trace.  To allow for less code
duplication, let the argument be a string providing the context
(e.g. "connection closed callback"), and then on an exception print
the context and the exception stack trace.
{{#dtdd name="onClose" type="Function"}}
Register a callback to be called when the session is closed.
When session reconnections are implemented, the client closing the

This comment has been minimized.

@n1mmy

n1mmy Nov 20, 2013

Member

Maybe condense both sections on "when there is session reconnect" into a single {{note}}?

id: "meteor_server_onconnection",
name: "Meteor.server.onConnection(callback)",
locus: "server",
descr: ["Register a callback to be called when a new DDP connection is made to the server."],

This comment has been minimized.

@n1mmy

n1mmy Nov 20, 2013

Member

Document returns a stop handle?

@@ -143,7 +143,8 @@ var toc = [
"Meteor.status",
"Meteor.reconnect",
"Meteor.disconnect",
"DDP.connect"
"DDP.connect",
"Meteor.server.onConnection"

This comment has been minimized.

@n1mmy

n1mmy Nov 20, 2013

Member

This should probably get aliased to Meteor.onConnection like publish, call, etc. See packages/livedata/server_convenience.js

The callback is called with a single argument, a `SessionHandle`.
The `SessionHandle` is an object containing the following fields:
<dl class="objdesc">

This comment has been minimized.

@n1mmy

n1mmy Nov 20, 2013

Member

Also document this.session (see email) in Meteor.methods section.

fn = Meteor.bindEnvironment(
fn,
function (err) {
Meteor._debug(

This comment has been minimized.

@n1mmy

n1mmy Nov 24, 2013

Member

Ditto String form.

@@ -0,0 +1,236 @@
var Fiber = Npm.require('fibers');
// like pollUntil but doesn't have to be called from testAsyncMulti.

This comment has been minimized.

@n1mmy

n1mmy Nov 24, 2013

Member

Er? Why not just use testAsyncMulti?

This comment has been minimized.

@awwx

awwx Nov 25, 2013

Contributor

testAsyncMulti currently has the limitation that the expect callback has to be called at the top level inside the test step function, you can't nest calls to expect inside one another. Since the tests didn't need the multiple steps of testAsyncMulti anyway, it seemed simpler to just extract the polling code.

onException = function (error) {
Meteor._debug(
"Exception in " + description + ":",
error && error.stack || error

This comment has been minimized.

@n1mmy

n1mmy Nov 24, 2013

Member

I like this idea! It will make a lot of a call sites much simpler.

awwx added some commits Nov 25, 2013

Add a "this.session" entry for methods in the docs.
Pull out the session object into its own documentation section.
Simplify callers of bindEnvironment by using the new string argument,
when the error callback only needs to print the exception.

Replace unsafe references to `err.stack` with
`err && err.stack || err`.  This avoids throwing a secondary exception
if the original exception in `err` isn't an object (`throw(null)` and
`throw(undefined)` are legal in JavaScript), and also displays the
error object if the stack trace wasn't included.
Remove sessionData.
Add tests to check that account data is cleaned up after a session
closes.

Make `establishConnection` available to account tests.

Remove code duplication between `poll` (now called `simplePoll`) and
the async_multi `pollUntil`.
@n1mmy

This comment has been minimized.

Member

n1mmy commented Nov 28, 2013

Looks great! Will merge. Nice work.

@timhaines

This comment has been minimized.

Contributor

timhaines commented Nov 28, 2013

Hey guys, just read up a little on this (not thoroughly). It looks like methods and publish functions are getting access to a server side session object that represents the DDP session? This is completely different to the Session on the client side, right?

If I've understood correctly, would this server side session object be a good place to hold the client's accepted languages, hostname, user agent, IP address etc? (for use within methods and publish functions)

@n1mmy

This comment has been minimized.

Member

n1mmy commented Nov 28, 2013

Yup!

@timhaines

This comment has been minimized.

Contributor

timhaines commented Nov 28, 2013

That's pretty damn exciting. Seems like a very nice approach.

sockjs/sockjs-node@c3970d0
#786

@n1mmy

This comment has been minimized.

Member

n1mmy commented Nov 28, 2013

Hrm. I ran our many-browser tests and this patch seems to cause issues. http://test-results-pr-1611-2-20131128-001934.meteor.com/

I've seen this pattern of failures (mostly accounts emails tests, some changing userid reruns subscriptions) before. In that case it was triggered by something closing the main DDP connection between the client and the server and it reconnecting mid-test. Could something in the new tests be causing a reconnect mid-test run?

I'll look into this early next week, unless you beat me to it @awwx =)

@awwx

This comment has been minimized.

Contributor

awwx commented Nov 29, 2013

That's strange. The hash-login-tokens PR does add some extra steps to the client login token tests that could be doing something, but the session-hooks tests are all server-only (making a DDP connection from the server to the server), so I don't know why it would be affecting the main DDP connection between the client and server. Unless of course one of the server tests is closing the main connection accidentally :) But I didn't see anything.

I'd need a way to reproduce this to investigate myself... are the many-browser tests something I could run? Or are the many-browser tests running multiple tests in parallel, and perhaps I'd be able to see the same thing if I run the unit tests in a loop from multiple browser windows?

@n1mmy

This comment has been minimized.

Member

n1mmy commented Dec 2, 2013

@awwx unfortunately, I can't give you access to the automated test repo. It's a private repo and has API keys and account details. But I think it should replicable without the cross-browser component -- I don't think the individual browsers make much of a difference. Deploying a test site (meteor test-packages --deploy foo.meteor.com) and pointing many browser tabs at it will hopefully do the trick.

@estark37

This comment has been minimized.

Contributor

estark37 commented Dec 2, 2013

@n1mmy @timhaines Unless I'm missing it, I think we still don't have the actual plumbing to get the IP/headers/etc. associated with a session, though session handles will certainly be part of that plumbing. Should we add that in?

Client IP seems like a good thing to add (and is part of the current login hooks design: https://meteor.hackpad.com/Login-hooks-design-notes-o0809sK58jX), but I'm not sure if HTTP headers is an elegant thing to have on a session, because it kind of intermingles DDP with HTTP. Maybe this is just a matter of naming the field nicely (like session.transportInfo = { transport: 'sockjs', headers: <http headers> } or something though that looks kind of clunky now that I write it out).

@timhaines

This comment has been minimized.

Contributor

timhaines commented Dec 2, 2013

@estark37 re: "not sure if HTTP headers is an elegant thing to have on a session" - you'd agree that it's beneficial to have at least a subset of the headers available from within methods and publish functions though right?

@estark37

This comment has been minimized.

Contributor

estark37 commented Dec 2, 2013

@timhaines Absolutely! Headers, query parameters, all sorts of things that are very HTTP-ish... I think it's just a matter of figuring out the right way to expose it that doesn't tie the concept of a DDP session to HTTP.

@awwx

This comment has been minimized.

Contributor

awwx commented Dec 2, 2013

Unless I'm missing it, I think we still don't have the actual plumbing to get the IP/headers/etc. associated with a session

Correct.

you'd agree that it's beneficial to have at least a subset of the headers available from within methods and publish functions though right?

Right, I suspect what we want is to have information derived from the headers (instead of necessarily the headers themselves).

For example, if the client is connecting to a load balancer which is forwarding connections to the Meteor server, the IP address of the client is available in the X-Forwarded-For header. On the other hand, if the client is connecting to the Meteor server directly, the X-Forwarded-For header must be ignored (since otherwise the client could forge it). So ideally the session would get the "client IP" supplied to it, without the developer using the session having to worry about how to dig it out of the headers etc.

@awwx

This comment has been minimized.

Contributor

awwx commented Dec 2, 2013

@n1mmy oops, http://test-results-pr-1611-2-20131128-001934.meteor.com/ got cleared. Do you happen to remember which test was failing?

I suspect running tests from multiple browser tabs (as opposed to multiple browsers) won't work because the accounts-password client tests login and logout. Firefox however has the option to run with different user profiles, so it's possible to run multiple instances of Firefox on a computer.

I tried running a bunch of simultaneous tests using both test-in-console and using multiple Firefox instances. I am seeing an expireTokens method error on the server, but it doesn't seem to cause a test failure for some reason, so I don't think that's the same one as you saw.

Exception while invoking method 'expireTokens' Error: Bad test. Must specify both oldestValidDate and userId.
I20131202-16:22:08.137(-5)?     at Object.Accounts._expireTokens (packages/accounts-base/accounts_server.js:211)
I20131202-16:22:08.137(-5)?     at Meteor.methods.expireTokens (packages/accounts-password/password_tests_setup.js:45)
I20131202-16:22:08.137(-5)?     at maybeAuditArgumentChecks (packages/livedata/livedata_server.js:1371)
I20131202-16:22:08.138(-5)?     at packages/livedata/livedata_server.js:545
I20131202-16:22:08.138(-5)?     at _.extend.withValue (packages/meteor/dynamics_nodejs.js:35)
I20131202-16:22:08.139(-5)?     at packages/livedata/livedata_server.js:544
I20131202-16:22:08.139(-5)?     at _.extend.withValue (packages/meteor/dynamics_nodejs.js:35)
I20131202-16:22:08.139(-5)?     at _.extend.protocol_handlers.method (packages/livedata/livedata_server.js:543)
I20131202-16:22:08.139(-5)?     at packages/livedata/livedata_server.js:443
@awwx

This comment has been minimized.

Contributor

awwx commented Dec 2, 2013

@n1mmy ack, I wasn't in the session-hooks branch :) Let me try again...

@awwx

This comment has been minimized.

Contributor

awwx commented Dec 2, 2013

@n1mmy when you run the many-browser tests, how many browsers are you typically running at once? Are you testing all packages?

@awwx

This comment has been minimized.

Contributor

awwx commented Dec 2, 2013

@n1mmy OK, this time I managed to checkout the session-hooks branch... :-) I ran 10 simultaneous phantomjs instances in a loop against test-in-console; and then 4 simultaneous instances of Firefox with test-in-browser hacked to keep running tests over and over; both running against accounts-base, accounts-password, and livedata tests.

No luck, I do get the expireTokens error message occasionally but no actual test failures. Without a successful reproduction I'm not getting much forwarder...

@mitar

This comment has been minimized.

Collaborator

mitar commented Dec 6, 2013

Great stuff! Two questions:

  • Is there a hook to hook into login step? So that when userId is set, you can additionally set something else into session? And when userId is unset to clear?
  • Will that be available through Meteor.connection() as well? In the same manner that Meteor.userId() is? In this manner it would be possible to use connection inside collection.deny and collection.allow?

I completely agree that browser headers and IP should be exposed in some way, if HTTP was used for DDP. Because this would be really useful for example to determine the language of the user, timezone, geo location and so on.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Dec 9, 2013

Merged in b5cd66d

@n1mmy n1mmy closed this Dec 9, 2013

@n1mmy

This comment has been minimized.

Member

n1mmy commented Dec 9, 2013

@mitar: yes, we'll have login hook eventually too. design ongoing at https://meteor.hackpad.com/Login-hooks-design-notes-o0809sK58jX. Also client IP eventually.

We still need to figure out how to make this available in more places, eg, allow / deny. Unfortunately, allow/deny callbacks take explicit positional arguments which make it hard to extend. We might want to make them take some sort of options hash to allow for extensibility.

@mitar

This comment has been minimized.

Collaborator

mitar commented Dec 9, 2013

@n1mmy: You could just provide Meteor.connection(). In general, Meteor.userId() works inside allow / deny anyway, so having that positional argument is a bit redundant.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Dec 9, 2013

Unfortunately, the name Meteor.connection is already taken on the client to mean the default DDP connection to the server. Having Meteor.connection mean one thing on the client and Meteor.connection() mean something else on the server sounds kinda confusing.

@mitar

This comment has been minimized.

Collaborator

mitar commented Dec 10, 2013

Oh, true. :-( But the name is not so important, the concept of having a function you call in a similar fashion as Meteor.userId(). But it is true that it should be named the same. So maybe it could be Meteor.serverConnection() and this.serverConnection inside publish?

BTW, but they are not so different concepts, no? On the client, Meteor.connection() gives you information about your connection to the server, on the server, Meteor.connection() gives you information to the client. In both cases you could store some additional information into it, which would preferably be synced automatically between client and server.

@mizzao

This comment has been minimized.

Contributor

mizzao commented Dec 17, 2013

This seems super useful. Time to rewrite https://github.com/mizzao/meteor-user-status...

However, the previous method of tracking connections allowed for reading the IP address. How can we do this with the Meteor.onConnection callback now?

@awwx awwx deleted the awwx:session-hooks branch Mar 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment