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

Overhauled presence to use the Meteor 0.6.7 connection API #18

Closed
wants to merge 3 commits into from
Closed

Overhauled presence to use the Meteor 0.6.7 connection API #18

wants to merge 3 commits into from

Conversation

nathan-muir
Copy link
Contributor

So, I was about to do a PR for https://github.com/nathan-muir/meteor-presence/tree/tail-call-timeout - but noticed -that Meteor 0.6.7 officially establishes the "connection" API - and decided to go a little crazy and rewrite presence to take advantage of it.

Features:

  • No more client managed sessionId [and race conditions associated] , a custom sessionKey is generated for each connection.
  • Configuration by Meteor.Presence.configure() instead of Meteor.Presence.state = and Meteor.settings.
  • Instant removal of presence on disconnect via connection.onClose
  • Instant registration of userId with presence - via auto-subscribe
  • Customisable client state function with options for value based isolation, and full reactive isolation
  • Customisable client side "heartbeat"
  • Server: Will clear all presences on startup - except if heartbeat is enabled on the server - allows for clusters of servers & removes presences created from servers that are no longer operating.

Incomplete/ Pending:

  • Unit Tests. Although the structure changes might make that easier
  • Server: Needs to restart presence for clients on the current server if the server manages to timeout (could be caused by database timeout)
  • Would like to update lastSeen even when state & heartbeat aren't use client side. Could get accurate time from _.debounce to the _.socket.on('data')

@dburles
Copy link
Collaborator

dburles commented Dec 17, 2013

Hey Nathan, thanks for the PR. Myself and Tom have been in the process ourselves of doing a re-write, taking advantage of the new connection api as well. You can see the work (in progress) over here: https://github.com/dburles/meteor-presence As you can see the code has been greatly simplified.

I've got a question in relation to your usage of a heartbeat, this was a consideration of mine as a way to deal with the lengthy duration that long polling clients remain connected. Obviously this is not ideal when we're wanting to display active users!

@dburles
Copy link
Collaborator

dburles commented Dec 17, 2013

I'm also quite interested in how you're handling multiple servers as this is something that i've not yet looked into

@nathan-muir
Copy link
Contributor Author

Haha, our results look very similar -

You're checking for Meteor.status().connected which is something I'm doing at an application level at the moment - I'll definitely add that to my PR

Otherwise, my branch has a few additions:

  • Client-side heartbeat, reactivity and isolate value options
  • The auto-subscribe will catch userId changes - even if the setPresence method isn't called
  • Not sending the connection.id to all clients (not sure if it matters - but thought a random id is safer?)
  • Optional server heartbeat to remove presences added by other servers- necessary so I don't have to clear presences on server startup which would break clustering

@nathan-muir
Copy link
Contributor Author

@dburles Missed the question there about client heartbeat.

This is just an optional setting - eg, if someone needs the lastSeen timestamp kept up-to-date.

edit: off-topic

@dburles
Copy link
Collaborator

dburles commented Dec 17, 2013

Interesting stuff. Though my thoughts on things like the lastSeen timer for example, raises the question of calling Meteor.disconnect() on idle which has an effect greater than what the core purpose of this package should really provide.

My idea on the ideal direction to take the package should be so that it provides a very solid battle-tested core that at least for now, feature-wise, does nothing more than provide the presence collection and allow clients to make use of the state function. Then from that point on extra features can be added if it makes sense to.

There are still some issues I've been seeing with my re-write surrounding dealing with clients connected through long-polling (they long time to drop off), as well as multiple-servers (untested), and possibly other edge cases.

So these parts are the low end stuff I've been trying to iron-out. Any ideas you have on that side would be great.

@dburles
Copy link
Collaborator

dburles commented Dec 17, 2013

Also on the question of publicising the internal connection/session id, I'm not sure at this point whether or not that is a bad idea

@nathan-muir
Copy link
Contributor Author

I seem to be great at misreading today - specifically talking about long polling eg. websocket fallback - the heartbeat implementation I'm using has no effect - all it does is call computation.invalidate() and force setPresence to be called.
(I somehow misread your commend as "issues with clients sitting and polling for a long time, even though they aren't active.", related to #6 )

What issues have you had with long-polling? I've just done some cursory testing meteor using DISABLE_WEBSOCKETS=1 and simulated latency, the connection.onClose is being called immediately.

@dburles
Copy link
Collaborator

dburles commented Dec 17, 2013

Oh right, rethinking it, the delay I experienced was most likely then from earlier when I was using the publish hack
I'll have to test again and double-check.

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

Successfully merging this pull request may close these issues.

None yet

2 participants