Skip to content
This repository has been archived by the owner on Aug 28, 2021. It is now read-only.

Generate common websocket messages on server #119

Open
1 task done
logan12358 opened this issue Jan 27, 2019 · 4 comments
Open
1 task done

Generate common websocket messages on server #119

logan12358 opened this issue Jan 27, 2019 · 4 comments

Comments

@logan12358
Copy link

Step 0:

Desired Behaviour

Some websocket messages should be sent by the server. The messages which this makes sense for are things like notification of a change on a stream or a project, or the addition of children to an object. This will make plugin implementations simpler and behaviour more consistent across plugins.

Existing broadcast and direct message forwarding could be retained, for cases where custom messages are needed.

Actual Behaviour

At the moment the server just forwards messages from clients to each other. Many of these messages are to notify other clients of updates to a stream. These messages could easily be generated on the server.

Affected Projects

Immediately, this affects SpeckleServer. Eventually, all projects with websocket communication could be upgraded.

Proposed Solution (if any)

Backward compatibility could be maintained, allowing existing clients to send custom messages. Then, all receivers could be upgraded to use the new server-generated messages. Then, senders could be upgraded to not send websocket messages.

I don't know if this has been discussed before, but I think it would be worth considering if it's not too difficult. If people generally approve, I can make a more detailed plan.

@didimitrie
Copy link
Member

Yep, this is something I was also considering, so by all means, let's do this (in a nice backwards compatible way, of course 😄 ).

My vague plan was to add a last extra middleware, something like emitNotification, in the routes that actually need to send a ws message (ie, not login, register, etc.). The controllers of those routes will then need to hoist into res the information about what event should be broadcast and in what "resource room", and then emitNotification takes care of the rest.

Ie:

r.post( '/streams', mandatoryAuthorisation, require( './streams/StreamPost' ), emitNotification )

// at the end of StreamPost, on the success path:
...
res.notification.eventRoom = `user-${req.user._id}` // note, rooms are no longer streamId-based, but resource based and there are checks on permissions when clients try and join them
res.notification.eventPayload = { message: "Stream created", streamId: streamId }
next() // will move into emitNotification (or whatever we'll call it)

// emitNotification will just broadcast, the ws api and redis take care of the rest.

I do have one issue before we embark on this, namely speckleworks/SpeckleRhino#226. It doesn't necessarily impinge on this one, but it's a bit more urgent.

This happens when, for example, you open the same gh defintion twice, resulting in conflicting clientIds - the server refuses to allow identical clientIds, and I feel some logic needs to be cleared up there beforehand; and am still not sure the best way to go about it (make it a client-side problem, or allow dupe clientIds on the server and add another sessionId for socket clients that's always unique and randomly generated on connection? Decisions, decisions...

@logan12358
Copy link
Author

Awesome!

One alternative to an express middleware would be a mongoose middleware. We could hook into the post-save hook for each schema. This would avoid repeating code when there are multiple endpoints performing similar operations.

That Rhino issue does seem a little thorny. This might be an extreme option: Do "clients" actually serve a purpose? They keep track of which application is used for each websocket connection, but not much beyond that. They aren't used for access control. They aren't persisted as the originators of objects or streams. Are those on the roadmap? Could we just completely remove the notion of clients and have websocket connections made with a user token and assigned a random ephemeral session id?

@didimitrie
Copy link
Member

Sorry for replying so late (i also should look at and merge the pr in the specs - haven't forgotten!!!).

I grok the post save hooks, and indeed it might be an easier and simpler option, as you basically have fewer points of entry so to say.

Do you think we could do things like diffs on the stream object array, see the modified fields, differentiate between create & update in the hook itself? If yes, perfect, it will save us trouble :) If no, i would go for the express middleware, as we have extra info in the route controllers that should make it easy to define the above. Maybe actually we can give it a shot like you propose and see where we get, and if needed we move to an express middleware.


The clients are important, as they also keep track of what data comes from what files, online/offline status, etc. They're a bit of a burden, i grant that, but they'll be the backbone of generating the "human network" graph of what data is sent from where, by whom and to where & whom and trace the informational flows enabled by speckle. Best diagram ever explaining this 🙈

The thorns of the rhino issue can be removed by allowing duplicate client ids on the server and sending a warning back to the client "yo, this shit is open in more places, by you and xxx". I also think there is a way to get a temporary, non-persistent client id (i forgot how, but the viewer, when not logged in does it). Having unique ids is important for automation issues, such as the slider in the browser control...

@logan12358
Copy link
Author

Alright! Digging into whether we can differentiate create and update on the mongoose side is on my todo list. I'll let you know what I find.

That makes sense about clients. Will reply with a small question on #120.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants