Bug 419327 - [r-OSGi] Support filtering event.topics to prevent serialization
Summary: [r-OSGi] Support filtering event.topics to prevent serialization
Status: RESOLVED FIXED
Alias: None
Product: ECF
Classification: RT
Component: ecf.protocols (show other bugs)
Version: 3.7.0   Edit
Hardware: All All
: P3 trivial (vote)
Target Milestone: 3.7.1   Edit
Assignee: Markus Kuppe CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-13 15:22 EDT by Wim Jongman CLA
Modified: 2013-10-23 10:49 EDT (History)
2 users (show)

See Also:


Attachments
Stacktrace of event distribution attempt (7.11 KB, text/plain)
2013-10-13 15:23 EDT, Wim Jongman CLA
no flags Details
Null for property value exception (5.62 KB, text/plain)
2013-10-13 15:25 EDT, Wim Jongman CLA
no flags Details
Filter event topics based on a system property before announced to remote (5.13 KB, text/plain)
2013-10-15 13:04 EDT, Markus Kuppe CLA
no flags Details
mylyn/context/zip (3.16 KB, application/octet-stream)
2013-10-19 03:46 EDT, Markus Kuppe CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Wim Jongman CLA 2013-10-13 15:22:22 EDT
https://sourceforge.net/p/r-osgi/bugs/175/

Eclipe E4 makes heavy use of the OSGi event admin. R_OSGi tries to distribute these events. Is there a way to switch this off? I don't want my local UI events to be distributed.

There is also a bug in the event handler (ChannelEndpointImpl#handleEvent). The code tries to copy the properties but the value of the property can be null which is not allowed for Hashtable

 Dictionary props = new Hashtable();
  for (int i = 0; i < propertyNames.length; ++i) {
      props.put(propertyNames[i], event.getProperty(propertyNames[i]));//NPE
    }
Comment 1 Wim Jongman CLA 2013-10-13 15:23:57 EDT
Created attachment 236436 [details]
Stacktrace of event distribution attempt
Comment 2 Wim Jongman CLA 2013-10-13 15:25:07 EDT
Created attachment 236437 [details]
Null for property value exception
Comment 3 Wim Jongman CLA 2013-10-13 15:26:30 EDT
Do we maintain the r_osgi code at ECF?
Comment 4 Markus Kuppe CLA 2013-10-13 15:53:25 EDT
(In reply to Wim Jongman from comment #3)
> Do we maintain the r_osgi code at ECF?

The SF project has been inactive for a while. My approach is to fix it at EF and send the patch to SF.
Comment 5 Markus Kuppe CLA 2013-10-13 16:41:49 EDT
Btw. this is partially related to bug #412186 "[Distributed EventAdmin] Add API for custom Serialization per event topic".
Comment 6 Scott Lewis CLA 2013-10-14 13:33:38 EDT
This bug is currently targeted for 3.7.0...which is supposed to occur today (Oct 14).  I/we are still waiting on this CQ approval http://dev.eclipse.org/ipzilla/show_bug.cgi?id=736

but want to ask...is a fix for this bug still anticipated for 3.7?
Comment 7 Markus Kuppe CLA 2013-10-14 13:38:38 EDT
(In reply to Scott Lewis from comment #6)
> but want to ask...is a fix for this bug still anticipated for 3.7?

Yes, actually I'm working on it right away and expect to commit after a break in 2-3 hours.
Comment 8 Markus Kuppe CLA 2013-10-14 13:40:10 EDT
Btw. we do not have to hold off the release if this doesn't get fixed. The problem does not affect the remote end and only causes the local r-OSGi code to throw a non serializable exception on every "org/eclipse/e4/*" topic event. This does not harm general functionality.
Comment 9 Scott Lewis CLA 2013-10-14 13:44:17 EDT
(In reply to Markus Kuppe from comment #7)
> (In reply to Scott Lewis from comment #6)
> > but want to ask...is a fix for this bug still anticipated for 3.7?
> 
> Yes, actually I'm working on it right away and expect to commit after a
> break in 2-3 hours.

Ok.   Since we don't have EF final approval on the CQ 736 yet anyway, I don't see why this can't go in for 3.7...as long as you are sure that it doesn't have any regression issues.
Comment 10 Markus Kuppe CLA 2013-10-14 16:43:31 EDT
This turns out to be more complicated that anticipated. Thus, deferring after 3.7.
Comment 11 Wim Jongman CLA 2013-10-14 16:50:12 EDT
(In reply to Markus Kuppe from comment #10)
> This turns out to be more complicated that anticipated. Thus, deferring
> after 3.7.

I had a simple idea to fix this:

Create a new property 

ch.ethz.iks.r_osgi.event.prop = event.remote

Then only events with this property are candidate for remote. It can be caught directly at the handleEvent callback.
Comment 12 Markus Kuppe CLA 2013-10-14 16:54:43 EDT
(In reply to Wim Jongman from comment #11)
> (In reply to Markus Kuppe from comment #10)
> > This turns out to be more complicated that anticipated. Thus, deferring
> > after 3.7.
> 
> I had a simple idea to fix this:
> 
> Create a new property 
> 
> ch.ethz.iks.r_osgi.event.prop = event.remote
> 
> Then only events with this property are candidate for remote. It can be
> caught directly at the handleEvent callback.

That completely reverses the current behavior of r-OSGi where all (known) topics are announced to the remote end and the remote is allowed to subscribe to each topic.
What you describe means all client code would have to be rewritten to set this marker property (unless I'm not getting your idea).
Comment 13 Wim Jongman CLA 2013-10-15 04:42:25 EDT
(In reply to Markus Kuppe from comment #12)

> 
> That completely reverses the current behavior of r-OSGi where all (known)
> topics are announced to the remote end and the remote is allowed to
> subscribe to each topic.
> What you describe means all client code would have to be rewritten to set
> this marker property (unless I'm not getting your idea).

Obviously, if the property is not available it would do what it does now.
Comment 14 Markus Kuppe CLA 2013-10-15 13:04:57 EDT
Created attachment 236494 [details]
Filter event topics based on a system property before announced to remote

Wim, does this patch fix the issue for you? You will have to create the topic filter via the "ch.ethz.iks.r_osgi.topic.filter" system property (e.g. "-Dch.ethz.iks.r_osgi.topic.filter=org/eclipse/e4/*")
Comment 15 Scott Lewis CLA 2013-10-15 16:29:26 EDT
We just got IP approval on CQ 7367

http://dev.eclipse.org/ipzilla/show_bug.cgi?id=7367

if you guys are ready WRT this bug then I'll do a release build and release ECF 3.7.  

Is this bug dealt with sufficiently for ECF 3.7.0?
Comment 16 Markus Kuppe CLA 2013-10-15 16:46:48 EDT
(In reply to Scott Lewis from comment #15)
> We just got IP approval on CQ 7367
> 
> http://dev.eclipse.org/ipzilla/show_bug.cgi?id=7367
> 
> if you guys are ready WRT this bug then I'll do a release build and release
> ECF 3.7.  
> 
> Is this bug dealt with sufficiently for ECF 3.7.0?

I do not feel confident in the patch. Lets not include it in 3.7.
Comment 17 Scott Lewis CLA 2013-10-15 17:03:27 EDT
(In reply to Markus Kuppe from comment #16)
> (In reply to Scott Lewis from comment #15)
> > We just got IP approval on CQ 7367
> > 
> > http://dev.eclipse.org/ipzilla/show_bug.cgi?id=7367
> > 
> > if you guys are ready WRT this bug then I'll do a release build and release
> > ECF 3.7.  
> > 
> > Is this bug dealt with sufficiently for ECF 3.7.0?
> 
> I do not feel confident in the patch. Lets not include it in 3.7.

Ok.  I'm currently waiting for EF final approval of just-submitted ECF IP log.  When I get that approval (hopefully afternoon/evening pacific time Oct 15), I'll do the release build and complete the release.
Comment 18 Markus Kuppe CLA 2013-10-16 13:22:26 EDT
(In reply to Markus Kuppe from comment #14)

> Wim, does this patch fix the issue for you? You will have to create the
> topic filter via the "ch.ethz.iks.r_osgi.topic.filter" system property (e.g.
> "-Dch.ethz.iks.r_osgi.topic.filter=org/eclipse/e4/*")

Wim, have you tried the patch?
Comment 19 Wim Jongman CLA 2013-10-18 04:14:57 EDT
> 
> Wim, have you tried the patch?

Trying now.
Comment 20 Wim Jongman CLA 2013-10-18 04:43:27 EDT
(In reply to Wim Jongman from comment #19)

Markus will this patch filter the topic that I don't want to remote?  Isn't this an undetermined list?
Comment 21 Markus Kuppe CLA 2013-10-18 05:00:48 EDT
(In reply to Wim Jongman from comment #20)
> (In reply to Wim Jongman from comment #19)
> 
> Markus will this patch filter the topic that I don't want to remote?  Isn't
> this an undetermined list?

This patch only filters the topics the user explicitly sets as part of the system property.
Comment 22 Wim Jongman CLA 2013-10-18 05:55:05 EDT
> 
> This patch only filters the topics the user explicitly sets as part of the
> system property.

Having to maintain a possibly endless list does not sound appealing to me. Or do you anticipate that it will not be that bad? Isn't it a better approach to filter based on an event property instead of the event topic? 

http://www.osgi.org/javadoc/r4v41/org/osgi/service/event/Event.html
Comment 23 Wim Jongman CLA 2013-10-18 05:58:13 EDT
BTW. In the current version of the chat client I don't see any of these mentioned stack traces any more no matter what I do with the UI (drag/minimize/resize). I get chat bot messages but there is no attempt to remote events any more. I'll try to find out what has been changed.
Comment 24 Markus Kuppe CLA 2013-10-18 07:17:38 EDT
(In reply to Wim Jongman from comment #22)
> > 
> > This patch only filters the topics the user explicitly sets as part of the
> > system property.
> 
> Having to maintain a possibly endless list does not sound appealing to me.
> Or do you anticipate that it will not be that bad? Isn't it a better
> approach to filter based on an event property instead of the event topic? 
> 
> http://www.osgi.org/javadoc/r4v41/org/osgi/service/event/Event.html

Use the filter "*" to ignore all events.
Comment 25 Wim Jongman CLA 2013-10-18 08:19:00 EDT
> Use the filter "*" to ignore all events.

I understand that, but what if I want to let a few topics through?
Comment 26 Markus Kuppe CLA 2013-10-18 08:53:02 EDT
(In reply to Wim Jongman from comment #25)
> > Use the filter "*" to ignore all events.
> 
> I understand that, but what if I want to let a few topics through?

Only filter org/eclipse/e4/*,org/eclipse/foo/*
Comment 27 Markus Kuppe CLA 2013-10-18 11:09:19 EDT
(In reply to Wim Jongman from comment #22)
> Isn't it a better
> approach to filter based on an event property instead of the event topic? 
> 
> http://www.osgi.org/javadoc/r4v41/org/osgi/service/event/Event.html

Hi Wim,

filtering on properties only will probably lead to undesired behavior if some event for a given topic are remoted while other are not.
Comment 28 Markus Kuppe CLA 2013-10-19 03:46:04 EDT
Fix pushed to master 010397b3629a85856aa61511a21786c034653a03 [1]

http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?id=010397b3629a85856aa61511a21786c034653a03
Comment 29 Markus Kuppe CLA 2013-10-19 03:46:09 EDT
Created attachment 236681 [details]
mylyn/context/zip