added sitemap event subscription mechanism

[kaikreuzer]

This PR provides the server side components for solving #640.

Note that it isn't yet ready for merging; it is still completely untested and probably very buggy.
Nonetheless, I want to give an early access to it, specifically for @resetnow to check whether it looks like the right approach to solve #640.

I specifically have one open issue: currently, I introduced a /rest/sitemaps/subscribe url that returns an SSE event stream upon a GET request. Within this request it creates a new subscription id. Imho this is not valid according to REST principles. There probably rather should be a POST /rest/events/subscribe request to create a new subscription, which is then followed by a GET /rest/sitemap/events/<subscriptionId>. WDYT?

Signed-off-by: Kai Kreuzer kai@openhab.org

[vlad-ivanov-name]

There probably rather should be a POST /rest/events/subscribe request to create a new subscription, which is then followed by a GET /rest/sitemap/events/.

Technically, it wouldn't be a problem to use this method as well the one that is already implemented; one thing to consider, though, is that two HTTP requests take a little bit more time than one. I don't know whether or not it matters here.

I'll try to adapt the client code and see how it goes.

[vlad-ivanov-name]

@kaikreuzer as of now, opening /basicui/app causes the following exception:

java.lang.NullPointerException
    at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:936)
    at java.util.concurrent.ConcurrentHashMap.containsKey(ConcurrentHashMap.java:964)
    at org.eclipse.smarthome.io.rest.sitemap.SitemapSubscriptionService.exists(SitemapSubscriptionService.java:112)
    at org.eclipse.smarthome.io.rest.sitemap.SitemapSubscriptionService.setPageId(SitemapSubscriptionService.java:143)
    at org.eclipse.smarthome.ui.basic.internal.servlet.WebAppServlet.service(WebAppServlet.java:146)

Do I need to apply any additional patches/changes?

[kaikreuzer]

@resetnow You need to make sure that subscriptionId is set in WebAppServlet.java:146, i.e. you have to pass it as a parameter (as it is set through req.getParameter("subscriptionId");).

one thing to consider, though, is that two HTTP requests take a little bit more time than one.

Right, but as this only needs to be done once for the session, this should not matter.
I'll then change the code to require an HTTP POST (in the coming days).

[kaikreuzer]

@resetnow I have updated this PR and was able to successfully subscribe to sitemap events.

What you need to do:

• Do an HTTP POST to POST /rest/sitemaps/events/subscribe. This results in an HTTP 201 (Created) with a location like http://localhost:8080/rest/sitemaps/events/dbda41b1-9680-4112-b837-fa9b83f651c7, where the last path segment is a newly created subscription id.
• Do an HTTP GET to http://localhost:8080/rest/sitemaps/events/dbda41b1-9680-4112-b837-fa9b83f651c7, which will be the connection for your SSE stream.
• Whenever you change the page (i.e. you call '/rest/sitemaps/{sitemapname}/{pageid}'), you should provide the subscription id as a query param (e.g. http://localhost:8080/rest/sitemaps/demo/0202?subscriptionid=dbda41b1-9680-4112-b837-fa9b83f651c7). - note that this is not applicable for the Basic UI; for this, you have to add the subscription id as a param to requests to the WebAppServlet - I already adapted the code to correctly switch the events to the new page then.

The events are similar to what you receive for the widgets when requesting a whole page, but carry less information (i.e. only what might have changed, but not the static data). Here is an example:

{
  sitemapName: "demo",
  pageId : "0202",
  widgetId: "02020102",
  label: "Temperature [23,0 °C]", 
  visibility: true, 
  item: {
    category : "temperature",
    label : "Temperature",
    name : "Temperature_Setpoint",
    state : "23",
    tags : [],
    type : "NumberItem"
  } 
}

[digitaldan]

Just wondering why there needs to be a stateful session that persists across requests? Could a request that contains the header 'accept: text/event-stream' to http://localhost:8080/rest/sitemaps/demo/0202 not return that sitemap fragment plus a stream of events for any updates?

[kaikreuzer]

@digitaldan This would not work for the Basic UI, since this does not at all request the sitemap pages through the REST API, but assembles the HTML directly on the server side.
As a result, the Basic UI would have create new SSE sessions all the time, while with the current implementation only a single SSE connection is required.

[digitaldan]

@kaikreuzer understood. It seems to be very specific to the basic ui impl or at least sitemaps, but does not follow the http://www.eclipse.org/smarthome/documentation/features/rest.html patterns so its hard to explain to new dvelopers why sitemaps has such a different eventing pattern compared to topics (which mostly are centered around items of course). My naive suggestion didn't follow those patterns either ;-) The pattern (step 1, step 2, step 3) seems awkward, but unfortunately I'm not offering any better solutions, I wish there was only 1 step ;-)

[kaikreuzer]

When creating the (main) SSE feature, I actually had in mind to allow changes to the subscription through REST calls (i.e. adding some additional topics), while keeping the existing SSE connection - so this PR here is pretty much in line with that idea (which was never implemented due to lack of time...). Also for the new sitemap concept, I would see a single SSE connection for the push-events with the possibility to re-configure the details of the subscription. I wouldn't call this design awkward 8-)

[vlad-ivanov-name]

@kaikreuzer thank you for the update. I was able to get subscription ID and open an event stream, however, there doesn't seem to be anything in this event stream. I see messages about creating new ID and opening a subscription in the console, but no actual events. I've pushed a few simple changes here, so you could cherry-pick aec4037. With this commit there should be a message in the browser console every time there is an event in the SSE stream.

Maybe I misconfigured a debug configuration? These messages do appear, though:

2016-09-01 21:11:05.904 [DEBUG] [i.r.s.internal.SitemapResource:210  ] - Created sitemap event subscription b755a3a2-37e1-4b9c-b4ae-1020e6be3b1b.
2016-09-01 21:11:06.009 [DEBUG] [i.r.s.internal.SitemapResource:234  ] - Client requested sitemap event stream for subscription b755a3a2-37e1-4b9c-b4ae-1020e6be3b1b.

[kaikreuzer]

@resetnow It seems to me that you missed "you have to add the subscription id as a param to requests to the WebAppServlet", at least I cannot see this in your code.

[vlad-ivanov-name]

@kaikreuzer I assume you mean requests that produce actual page html, so the subscription location (the page user is currently on) could be updated? It would be good to initialize location to something by default without the need to make additional request (essentially refreshing the page). It could be either referrer header used to set location or a separate request sent once UI is loaded.

I have added the latter method (as it is easier to implement) in 39c1c8d. It seems the request is made and location is updated, but I still don't see any events coming from the stream.

[kaikreuzer]

I assume you mean requests that produce actual page html

Yes, correct.

It would be good to initialize location to something by default

Ok, I have just pushed this commit, which now allows to provide a sitemap and a pageid as query params to the initial SSE GET request (like GET /rest/sitemaps/events/424c6a8b-66cd-4f4d-b137-ba8344025a10?sitemap=demo&pageid=0202)- this then automatically sets the initial page. Additionally I have added logging.

I am now only doing the two calls:

POST http://localhost:8080/rest/sitemaps/events/subscribe
GET /rest/sitemaps/events/424c6a8b-66cd-4f4d-b137-ba8344025a10?sitemap=demo&pageid=0202

and see this in the logs:

2016-09-03 00:37:32.372 [DEBUG] [r.s.SitemapSubscriptionService:94   ] - Created new subscription with id 424c6a8b-66cd-4f4d-b137-ba8344025a10
2016-09-03 00:38:00.946 [DEBUG] [r.s.SitemapSubscriptionService:155  ] - Subscription 424c6a8b-66cd-4f4d-b137-ba8344025a10 changed to page 0202 of sitemap demo
2016-09-03 00:38:05.139 [DEBUG] [i.r.s.internal.SitemapResource:239  ] - Client requested sitemap event stream for subscription 424c6a8b-66cd-4f4d-b137-ba8344025a10.

When I now switch something in the Basic UI on the Widget overview page, I receive events (and according log entries):

2016-09-03 00:38:23.147 [DEBUG] [r.s.SitemapSubscriptionService:52   ] - Sent sitemap event for widget 02020100 to subscription 424c6a8b-66cd-4f4d-b137-ba8344025a10.

I have added the latter method (as it is easier to implement) in 39c1c8d

I guess you won't need this anymore.

but I still don't see any events coming from the stream.

Did you remember to change some states? :-) I hope the logging will help you to get it working!

[vlad-ivanov-name]

Thanks! I have updated resetnow/smarthome/ssesitemaps. It works for me, there is a problem, though: after refreshing a page several times every state change will cause multiple events to be sent. I have added a console.log call for event reception, so it's possible to see it in browser. By the way, is there is any kind of “garbage collector” for unused subscriptions?

Also, do sitemap events also work for longpolling or do we keep longpolling as it is?

[kaikreuzer]

I have updated resetnow/smarthome/ssesitemaps

I can not pull these changes in my branch as you didn't sign-off your commits. Could you create a PR against my branch, then I could simply merge that to include your changes.

after refreshing a page several times every state change will cause multiple events to be sent

Do you create a new subscription on every page refresh? Or are these duplicate events on a single subscription (didn't yet have the time to test your code myself, sorry). If the latter, I'll look into it to fix it.

is there is any kind of “garbage collector” for unused subscriptions?

Not yet, but this is clearly something I need to implement, before the PR gets merged. But I first wanted to see the feature working at all :-)

Also, do sitemap events also work for longpolling or do we keep longpolling as it is?

Imho, it does not make any sense to combine SSE with longpolling here. A client should do either or.

[vlad-ivanov-name]

Do you create a new subscription on every page refresh? Or are these duplicate events on a single subscription?

Both. I do create a subscription on initial page load and therefore on refresh, and there are duplicate events on a single subscription.

[vlad-ivanov-name]

kaikreuzer#5

[kaikreuzer]

Just tried your changes, but although I see web/smarthome.js changed in my workspace, the Basic UI does not seem to subscribe to anything - there also isn't a subscriptionId added to the url when navigating. Are you sure that you included all necessary changes...?

[vlad-ivanov-name]

Well there should be at least a subscription request which you should see in the developer tools of your browser in the “network” tab. If there isn't one, try clearing the browser cache — I checked the files I have pushed and they appear to be correct (in the commit above).

Also, are there any errors in the javascript console? I've tried Firefox and Chrome, other browsers might not work.

[kaikreuzer]

Ok, my fault, I wasn't running the correct code.
Now I see a subscription being created, the SSE stream being requested and the page being changed.

Nonetheless, I do not see any reaction of the UI when a value is changed and I do not get any logging int he JS console...

I do create a subscription on initial page load and therefore on refresh

Is there any way to avoid this? If there is an existing SSE connection, could you just continue to use it?

[vlad-ivanov-name]

Is there any way to avoid this? If there is an existing SSE connection,
could you just continue to use it?

I could use persistent browser storage.

Something like this:

if (connect(savedSubscriptionId) == error) {
requestNewSubId();
reconnect();
}

I assume there will be an error when the subscription has already been
cleaned up by the garbage collector.

[kaikreuzer]

I assume there will be an error when the subscription has already been
cleaned up by the garbage collector.

What I meant is to know whether there is an active SSE connection, not if there is subscriptionId. Or is the SSE connection in any case closed when you do a refresh?

[vlad-ivanov-name]

Yes, it is closed on page refresh.

[kaikreuzer]

Hm, ok, so the garbage collection is urgently needed... I didn't yet find any mechanism to notice when a client closes the SSE connection, though. Will have to figure out how to do this.

But apart from this, do you have any idea, why the UI doesn't receive the events? In the log, I see "Sent sitemap event..." for the correct subscription id...

Another remark: Having the subscriptionId as an HTTP GET parameter causes a problem for bookmarks etc. as it causes an IAE, if the subscription does not exist (anymore). Do you have any good idea how to circumvent this? I wonder, if a second HTTP request (a PUT) would actually be the cleaner option...

[vlad-ivanov-name]

I didn't yet find any mechanism to notice when a client closes the SSE connection, though.

It's probably safe to assume that SSE connection can be dropped by remote without any prior notification (wifi can become out of range, cable can be unplugged, etc). The only way to detect it is to send keep-alive messages every minute or so. This message may be a : comment or just a different type of event (event: keepalive won't trigger a client-side event handler). The subscription may be considered obsolete when a socket write fails.

Having the subscriptionId as an HTTP GET parameter causes a problem for bookmarks etc. as it causes an IAE, if the subscription does not exist (anymore). Do you have any good idea how to circumvent this? I wonder, if a second HTTP request (a PUT) would actually be the cleaner option...

It's possible to display an URL without subscriptionId in the address bar and make requests w/ one when navigating.

But apart from this, do you have any idea, why the UI doesn't receive the events? In the log, I see "Sent sitemap event..." for the correct subscription id...

https://resetnow.ru/temp/esh-screencast.webm

The only thing I could suggest is clearing browser cache. Also, if you're on linux or mac, try running wget <sub url here> -O - and changing states, there should be events (event: event, etc).

If nothing helps we can schedule a “live” debugging session via IRC, Telegram or anything else that works on Linux. I am available 20:00 to 0:00 (Moscow, UTC+03:00, GMT+04:00).

[kaikreuzer]

Thanks for the hints, I'll give it a go. Thanks for the offer for a debugging session, but I think I'll first try to dive deeper myself!

[kaikreuzer]

Hi @resetnow! I have no clue what were my problems the last time, but now the Basic UI correctly receives the events.
I have also added a clean up of closed SSE connections - so there should be no memory leaks.

The only issue that remains is that the widgets are not updated - but I guess this is the part you didn't implement yet :-)
Could you please try out the latest version and add what needs to be done on the Basic UI side?

[vlad-ivanov-name]

Hello @kaikreuzer and thanks for the update. Glad to hear it's working for you, were you able to reproduce the issue where one event is sent to a subscription multiple times?

The only issue that remains is that the widgets are not updated

If you mean that widgets visibility is not changing then yes, I haven't added it yet. Otherwise controls should be toggling and labels changing, as in the video I shared before.

(edit: the video link was dead, I have updated it)

[kaikreuzer]

@resetnow Ok, I think I finally solved it - there should be no duplicate events anymore!

@maggu2810 From my pov this is ready to be reviewed now.

[wborn]

I made a local build of this PR by merging kaikreuzer:ssesitemaps because I think it should fix my _#_1 Basic UI nuisance. I do see item values properly getting formatted and transformations properly applied. 👍

But the following things do not update when values change:

• colors specified with valuecolor
• images
• group values, e.g.:
  Group:String:OR("true", "false") Motion "Motion detected [(%d)]" <motion>

When I reload the page they do get updated, but I think that should be fixed with this PR?

I hope it is just me and that I made an error somewhere. I have tested with Chrome/Chromium/Firefox on Ubuntu 16.04. In all browsers I see the same behavior. Also I have forced refreshing browser caches.

[vlad-ivanov-name]

@wborn you didn't do anything wrong, things you listed are still missing. The one I can fix is visibility state, don't really know about the others.

[wborn]

@resetnow OK thanks for letting know that there are still some limitations. Now I know what I can test. I am already happy that the days are numbered for my formatting workarounds.

[kaikreuzer]

images

Do you refer to charts here? There are indeed no events for them yet.

[wborn]

No I mean the icons of items. But I have just tested how things work with the demo instead of my own setup and it works a lot better there. :-) I now do see icons/sliders/toggle update correctly.

However after a while it seems the eventsstream stops for some reason. When that occurs the icons no longer update. Neither do other things.

I can reproduce it on the demo by:

1. Open Basic UI
2. Open Cellar
3. Toggle several switches
4. Click the back button in top left corner
5. Open Garden
6. Toggle several switches
7. Click the back button in top left corner
8. Repeat from step 2.

Reloading the browser does not fix it. So I think it is something in the backend. I don't see any Java stacktraces or things in the browser console.

When it works, it works really well though!

[kaikreuzer]

Thanks a lot @wborn, with this it was easy to reproduce and to fix.
I also added support for group items, they are also now updated correctly (with the one exception that numbers in labels (no of lights = 3) do not change if a single light changes, since the group item state does not change (it stays on). This is not related to this PR, though, so I won't further look into that part).

@maggu2810 I would declare to be done with this feature now and would suggest it to be reviewed and merged. If there are any further functional issues / missing features, I'd prefer to deal with them in follow-up issues.

[wborn]

@kaikreuzer thanks for fixing this issue. I can confirm it is fixed. Also on my own big sitemap things work well now! I indeed saw the SUM AVG groups properly update now. I think a lot of people will be happy when this PR is reviewed and merged. :-)

[maggu2810]

I assume an event occurs more often then add / remove callback.
Would it be better for performance if we create the "no duplicate set" on every add / remove call?

[maggu2810]

If we don't care about the above comment, we should create the set only once before the for-each-event loop and not in every iteration.

[kaikreuzer]

Addressed by #2288.

[maggu2810]

Thanks

[maggu2810]

Regardless of the comment above I will merge this PR now.
It is no blocker, only an improvement.

But please comment it that it does not get lost.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/basic-ui-live-update/39514/7

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/sse-not-seeing-visibility-changes-in-sitemap/97800/12

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/sse-not-seeing-visibility-changes-in-sitemap/97800/14