[jitsi-dev] [jitsi-videobridge] flesh out long-polling support in the rest api and add a endpoint exp… (#106)


#1

…iration message. NEEDS CLEANUP.

There's some ugly, temporary hacks here; I'd consider this basically a straw man implementation for the purpose of review. Hopefully the basic ideas are sound, but I figured I'd just get this review going in the background so the team can take a look and we can get it cleaned up and committed upstream. Mainly, need input on the mechanism by which the core code will push events to the long-polling channel--ideally this would be the same as how it's done with xmpp.

@lyubomir

NOTE: re-opened this as I accidentally did the previous one from the wrong branch.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/jitsi-videobridge/pull/106

-- Commit Summary --

  * flesh out long-polling support in the rest api and add a endpoint expiration message. NEEDS CLEANUP.

-- File Changes --

    M pom.xml (5)
    M src/main/java/org/jitsi/videobridge/Conference.java (27)
    M src/main/java/org/jitsi/videobridge/Endpoint.java (6)
    M src/main/java/org/jitsi/videobridge/rest/LongPollingServlet.java (73)
    M src/main/java/org/jitsi/videobridge/rest/RESTBundleActivator.java (5)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/106.patch
https://github.com/jitsi/jitsi-videobridge/pull/106.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106


#2

@@ -38,7 +59,55 @@ protected void doGet(
         throws IOException,
                ServletException
     {
- // TODO Auto-generated method stub
- response.setStatus(HttpServletResponse.SC_NOT_FOUND);
+ System.out.println("BB: IN LONG POLLING");
+ Continuation continuation = ContinuationSupport.getContinuation(request);

The last bit of information on the subject of asynchronous processing of HTTP requests that we exchanged via e-mail was:

We’ll use asynchronous processing of HTTP requests defined by the Servlet 3.0 specification. Please refer to https://docs.oracle.com/javaee/6/api/javax/servlet/ServletRequest.html#startAsync(javax.servlet.ServletRequest, javax.servlet.ServletResponse).

I know I mentioned Jetty Continuations in person prior to that e-mail exchange when we were still in the support investigation phase. I'd like us to go with the Servlet 3.0 API because we have it already in our dependency list (while Jetty Continuations is yet another dependency) and is equally easy to work with.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106/files#r46614262


#3

@@ -38,7 +59,55 @@ protected void doGet(
         throws IOException,
                ServletException
     {
- // TODO Auto-generated method stub
- response.setStatus(HttpServletResponse.SC_NOT_FOUND);
+ System.out.println("BB: IN LONG POLLING");
+ Continuation continuation = ContinuationSupport.getContinuation(request);
+
+ // We don't get the target here (TODO: is that something that could
+ // be fixed?), so we need to do a bit more work to extract the bits of the path
+ // we're interested in.
+ // The stuff we're interested in starts after the /colibri/
+ String colibriPath = request.getRequestURL().toString().split("colibri/")[1];
+ String target = colibriPath.split("/")[0];
+
+ continuation.suspend();

Starting an asynchronous processing of the HTTP request here is too early because you don't even know whether the target is supported and is to be asynchronously processed.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106/files#r46615042


#4

@@ -38,7 +59,55 @@ protected void doGet(
         throws IOException,
                ServletException
     {
- // TODO Auto-generated method stub
- response.setStatus(HttpServletResponse.SC_NOT_FOUND);
+ System.out.println("BB: IN LONG POLLING");
+ Continuation continuation = ContinuationSupport.getContinuation(request);
+
+ // We don't get the target here (TODO: is that something that could
+ // be fixed?), so we need to do a bit more work to extract the bits of the path
+ // we're interested in.
+ // The stuff we're interested in starts after the /colibri/
+ String colibriPath = request.getRequestURL().toString().split("colibri/")[1];
+ String target = colibriPath.split("/")[0];
+
+ continuation.suspend();

Once you've identified a target/HTTP request for asynchronous processing and started it i.e. suspended the request, you are to register the continuation (either Jetty Continuations or Servlet 3.0 API) with an asynchronous service which is to resume the continuation when the waited-for event occurs. In other words, the code bellow should return the control to the calling method as soon as possible and without blocking.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106/files#r46615663


#5

+ if (target.equals("subscribeToEvents"))
+ {
+ String confId = colibriPath.split("/")[1];
+ System.out.println("BB: subscribing to conference events for conference " + confId);
+ Conference conference = videobridge.getConference(confId, null);
+ if (conference == null)
+ {
+ System.out.println("Couldn't find conference " + confId);
+ }
+ else
+ {
+ System.out.println("found conf object");
+ System.out.println("Waiting for event");
+ try
+ {
+ JSONObject pushEvent = conference.getPushEvent();

That's a blocking method call and is not the way to go with asynchronous processing (as commented above). What you want to do is suspend the request, register the suspension/continuation with another entity which monitors the blocking queue and resumes the continuation when at least one element is queued. When the request is resumed, read from the queue without blocking.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106/files#r46616253


#6

@@ -1673,4 +1682,22 @@ public String getName()
     {
         return name;
     }
+
+ public void addPushEvent(JSONObject event)
+ {
+ System.out.println("CONF ADDING PUSH EVENT");
+ try {
+ pushEventQueue.put(event);

Limiting the number of events stored in the queue is probably one of the issues that we'll want to look into sooner rather than later; otherwise, we're opening ourselves to running out of memory. We could do a maximum number of events, timing events out, etc.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106/files#r46616737


#7

@@ -1673,4 +1682,22 @@ public String getName()
     {
         return name;
     }
+
+ public void addPushEvent(JSONObject event)
+ {
+ System.out.println("CONF ADDING PUSH EVENT");
+ try {
+ pushEventQueue.put(event);

yeah i agree...a max size that returns some value on failure (so the caller can potentially try again) seems good. but at a higher level, this queue (especially in this location) was really just a hack. doesn't the conference already have a mechanism for pushing events out on the xmpp channel? i'd think there should be some code we can re-use from there

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106/files#r47175109


#8

@@ -38,7 +59,55 @@ protected void doGet(
         throws IOException,
                ServletException
     {
- // TODO Auto-generated method stub
- response.setStatus(HttpServletResponse.SC_NOT_FOUND);
+ System.out.println("BB: IN LONG POLLING");
+ Continuation continuation = ContinuationSupport.getContinuation(request);

i think that i did look into the async context stuff, but ended up going down the continuation route--i don't remember what the reason was. but i'll work on moving it over to the async servlet stuff.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106/files#r47175352


#9

@@ -38,7 +59,55 @@ protected void doGet(
         throws IOException,
                ServletException
     {
- // TODO Auto-generated method stub
- response.setStatus(HttpServletResponse.SC_NOT_FOUND);
+ System.out.println("BB: IN LONG POLLING");
+ Continuation continuation = ContinuationSupport.getContinuation(request);
+
+ // We don't get the target here (TODO: is that something that could
+ // be fixed?), so we need to do a bit more work to extract the bits of the path
+ // we're interested in.
+ // The stuff we're interested in starts after the /colibri/
+ String colibriPath = request.getRequestURL().toString().split("colibri/")[1];
+ String target = colibriPath.split("/")[0];
+
+ continuation.suspend();

yep, sounds good. again this was more just shortest-path to get things working.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106/files#r47175409


#10

@lyubomir finally got some time to work on this. still need some cleanup, but let me know if this is what you had in mind.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106#issuecomment-173299349


#11

Can one of the admins verify this patch?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/106#issuecomment-192016137