1144851 - "status" action for POST /rooms/{token} is too restrictive

Status: RESOLVED FIXED

(Hello (Loop) :: Server, defect)

Description

[Mark Banner (:standard8)]

I've been trying to write the client implementation for posting status data per bug 1137813. Unfortunately, the validation implemented in bug 1137849 is too restrictive for what we need.

Some of the issues appear to be that we're trying to indicate the new state after an event has happened, but the server doesn't allow this.

Here's some of the issues I've noticed:

- We can't do a "Publisher.streamCreated" for state "sending".

- "Session.connectionCreated" for state "sending"
-- note our implementation has the client setting up sending streams before the session has both ends connected, looking at the wiki page, the "sending" state seems best to represent this

- "Session.streamCreated" for state "sendrecv"
-- This is for joining the initial connections, but also, we need to be able to have more than one stream added, e.g. in the case of screen sharing.

- "Session.streamDestroyed" in state "sending"
-- This is where the remote stream is destroyed and we end up in the sending state.


Here's a few extracts from objects we're sending to the server. Note these have the "action" parameter added to them after the point where I took these from the logs.

OK   - { event: "Session.connectionCreated", state: "waiting", connections: 1, sendStreams: 0, recvStreams: 0 }
Fail - { event: "Publisher.streamCreated", state: "sending", connections: 1, sendStreams: 1, recvStreams: 0 }
Fail - { event: "Session.connectionCreated", state: "sending", connections: 2, sendStreams: 1, recvStreams: 0 }
Fail - { event: "Session.streamCreated", state: "sendrecv", connections: 2, sendStreams: 1, recvStreams: 1 }
Fail - { event: "Publisher.streamCreated", state: "sendrecv", connections: 2, sendStreams: 2, recvStreams: 1 }
OK   - { event: "Publisher.streamDestroyed", state: "sendrecv", connections: 2, sendStreams: 1, recvStreams: 1 }
Fail - { event: "Session.streamDestroyed", state: "sending", connections: 2, sendStreams: 1, recvStreams: 0 }
OK   - { event: "Session.connectionDestroyed", state: "sending", connections: 1, sendStreams: 1, recvStreams: 0 }
OK   - { event: "Session.connectionDestroyed", state: "cleanup", connections: 1, sendStreams: 1, recvStreams: 1 }

Comment 1

[Mark Banner (:standard8)]

It also looks like we can't log the final Session.connectionDestroyed for the standalone UI - because we're not FxA authorised, we're relying on the sessionToken, and that appears to be invalid after the "leave".

I might be able to make that work synchronously, and get it in before the "leave", but I'd prefer not to if possible.

Comment 2

[Mathieu Leplatre [:leplatrem]]

Ok, it looks like I misunderstood the meaning of the state machine diagram concerning validation.(https://wiki.mozilla.org/Loop/Architecture/Rooms#Updating_Session_State)
Shall we validate specific combinations of (state, event) at all ? Or was it just some missing bits on the specs diagram ?


Regarding the final state, I'll investigate and see if something like adding a condition on session token validation is feasible.

Thanks for your report!

Comment 3

[Adam Roach [:abr]]

(In reply to Mathieu Leplatre (:mathieu) from comment #2)
> Ok, it looks like I misunderstood the meaning of the state machine diagram
> concerning
> validation.(https://wiki.mozilla.org/Loop/Architecture/
> Rooms#Updating_Session_State)
> Shall we validate specific combinations of (state, event) at all ? Or was it
> just some missing bits on the specs diagram ?

I won't have time to dig into this for a while, but I did want to comment on this (I'm not clearing the ni because I plan to come back when I have more time). I wasn't expecting the server to perform any validation here -- I had envisioned this as a simple logging function on the server end.

In terms of transitions not shown, note in particular that there are quite a few "an event occurs that leaves us in the same state but simply increments or decrements a count" transitions that are left off for the sake of making the diagram more usable.

Comment 4

[Alexis Metaireau (:alexis)]

Thanks for the response Adam.

Just to clear that out: Is your suggestion we should remove all the validation logic on the server side for this feature?

Comment 5

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #4)
> Thanks for the response Adam.
> 
> Just to clear that out: Is your suggestion we should remove all the
> validation logic on the server side for this feature?

Yes, please. Not just for transitions, but for state names as well: if we need to change the state diagram, I'd like to be able to do so without waiting for a new server deployment.

Thanks!

Comment 6

[Mathieu Leplatre [:leplatrem]]

Attachment: PR 323

Comment 7

[Alexis Metaireau (:alexis)]

Comment on attachment 8581566 [details] [review]
PR 323

https://github.com/mozilla-services/loop-server/commit/64d0ab6cf7731b585d59e16d227766b44de2659a

It's merged, though this version doesn't allow posting anything after the "leave" event.

Adam, would that be okay if we don't do any authentication check for the events sent after the "leave" event?

Comment 8

[Mark Banner (:standard8)]

Thinking about status after leave, I think I could either make it so that we wouldn't send status, or we could just say that "leave" is equivalent to the final status.

If both are getting logged in the same place, then I would think that would be a reasonable assumption.

This would also mean that we wouldn't need any other changes on the server.

Comment 9

[Rémy Hubscher (:natim)]

> If both are getting logged in the same place

Yes they are

> This would also mean that we wouldn't need any other changes on the server.

What are the other information that you want to send on the leave action?

Comment 10

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #8)
> Thinking about status after leave, I think I could either make it so that we
> wouldn't send status, or we could just say that "leave" is equivalent to the
> final status.
> 
> If both are getting logged in the same place, then I would think that would
> be a reasonable assumption.
> 
> This would also mean that we wouldn't need any other changes on the server.

For the sake of being able to quickly answer the questions that we know we have, I'm okay with this for now. If we end up with questions that need post-leave data to answer, we may need to revisit this issue.

Comment 11

[Alexis Metaireau (:alexis)]

Thanks then, closing.