Community
Participate
Working Groups
Currently, we don't check if we are on the UI thread when we call into JFace or SWT methods, but we should. The main issue is how can we implement this in a way that does not tie DataBindingContext/ValueBinding/CollectionBinding to SWT...
One idea: Perhaps this should be implemented inside the SWT updatables? We could have a standard check at the beginning of each public method that calls itself on an async runnable if it discovers it's not running on the UI thread?
Boris, Given that org.eclipse.jface.databinding has org.eclipse.swt as a dependent of it, why can't we put code that just creates runnables and either runs them immediately or as a Display.syncExec ? This is only required in code that already imports SWT packages as it would be before we called anything that updated SWT, or is the problem that we might implicitly call SWT because we are doing a notification without any knowledge of who is listening to it, and the listener callback method is the one that breaks ?
Yes I agree with Dave that this should be implemented inside the SWT updatables. The mechanism (running a Runnable directly, or using Display.asyncExec if needed) could be put into a common superclass. Joe, when you implement this, make sure that you wrap the whole method body of setValue() and similar methods inside the Runnable, i.e. including the try..finally and the fireChangeEvent call.
This is actually tricky to implement. The problem is that we need code that queries the result of UI components and then deals with them, so we have to async exec these on the Display thread and then wait for the result before we can continue in the framework code. In the JUnits we need to be able to invoke non-UI thread code that updates the model and then tests the GUI on the UI. This means the JUnit has to wait for the non-UI thread code to finish, however the non-UI thread invokes async stuff on the UI and the two deadlock. I can't see how we can have non-UI thread code waiting on the UI to finish an async and then return the resuilt, because the non-UI waits on the UI, and for the JUnits the UI thread waits on the non-UI.
This is now done with two new helper classes. SyncRunnable and AsynRunnable. There are tests for all of the updatablevalues to show that the model can change in a non-UI thread and the UI is updated OK and the code is released. The only thing I didn't change was TreeViewerUpdatableCollection because I can't see a test case at the moment that shows that the tree refreshes when the model does that I can easily modify to test for non UI thread conditions.
Gili - you're working on Tree support so I'm reassigning this for you to close out when you're happy that Tree works for non-UI thread changes.
Gili, can you please check this and close if applicable? I know you're busy, but if you could handle this one thing I'd be most grateful. Thanks.
This have been done a while ago by Joe, and I have added it to the trees/combos
We are backing out of this for the refactored version - our old approach was too naive and may easily lead to deadlocks.
Our current story is that the data binding framework is not thread-safe (it never was), and that everything has to be done on the UI thread. It would be good to get rid of this restriction, but we would have to better understand the implications of going multi-threaded. Marking as 3.2 as a reminder to look at this problem again soon.
I found a good description of (some of) the problems with using the Observer pattern in a concurrent environment: http://www.openoffice.org/servlets/ReadMsg?list=dev&msgNo=2784 It is still unclear to me if there is a solution whereby concurrent observers can make use of incremental updates rather than having to read the complete current state of the subject (==IObservable) whenever they are notified about a change. Maybe Doug Lea's concurrency book has some answers, or pointers?
(In reply to comment #11) > Maybe Doug Lea's concurrency book has some answers, or pointers? Unfortunately not, the sample chapter linked below is the one that discusses the Observer pattern: http://www.awprofessional.com/articles/article.asp?p=31539&seqNum=5&rl=1
Created attachment 52092 [details] work in progress This patch is not complete in any way, I am just capturing my current state before submitting to a build. The basic idea is to have one or more "Realms" (any suggestions for a better name?) which are an abstraction of SWT's single thread model. Each observable belongs to a certain realm and can only accessed from within that realm. Mapped to SWT the previous sentence can be read as: SWT observables belong to the UI thread and can only accessed from the UI thread. Likewise, observables only fire events within their realm (read: on the UI thread). The abstraction I am thinking of also allows for realms that are based on holding a lock instead of being on a particular thread. This structure makes programming observables easy since by definition they don't have to worry about concurrency, and it makes implementing bindings hard because they now have to be able to bind observables from two different domains. I believe that this is a good thing because there will only be a few binding implementations (ValueBinding, ListBinding, SetBinding, ...) but many observable implementations. Many thanks to John Arthorne and Stefan Xenos for discussing this with me. Keep your fingers crossed that this will solve the problem! :)
Created attachment 52570 [details] work in progress I have released this into a branch called Bug116920_investigation (for the three main plug-ins).
I took a look at the patch and wanted to voice a concern about the API. The change forces concurrency into every use case for observable construction rather than just for the use cases that need it (all constructors have been changed to accept a Realm). Can we not accomplish this via decorators or some other means? I can elaborate if necessary. My main fear is that we're forcing the edge case to be addressed by every use case, even when it's not a concern. Even if there was an easy way to create Realms it seems like it's overkill and makes things a little harder to use. I like the usability of classes like WritableValue as I get change notifications without any code or thought. My other concern is that this leads to more code to just get something up and running. A developer now has to make a decision about concurrency for every observable. It's not a bad thing to think about concurrency but it doesn't mean it will done correctly when forced.
(In reply to comment #15) > The > change forces concurrency into every use case for observable construction > rather than just for the use cases that need it (all constructors have been > changed to accept a Realm). Can we not accomplish this via decorators or some > other means? I can elaborate if necessary. Yes, please elaborate. As far as I know, concurrency cannot be added as an afterthought, and it is one of those cross-cutting concerns that affect the shape of an API overall, rather than being something you can solve by adding something on the side. If you could prove me wrong that would be great.
From what I can tell the Realm is only used by the binding. It seems like the realm is only set on the observable to associate the realm with the observable to later be accessed by the binding or any outside entity. So for the use cases that need realms the following will associate a realm with the observable to be later retrieved by the binding. IObservableValue observable = new WritableValue(String.class); observable = Realms.associateRealm(new Realm(), observable); Realms.associateRealm(...) would provide a decorator that is merely a pass thru to the observable but the decorator will also implement IRealmable (name is just for demonstration purposes). public interface IRealmable { public Realm getRealm(); } The binding would then perform an instanceof on the observable for IRealmable and run the runnable in the appropriate context. This would allow for the binding to still access the realm but this is allowed to be a decision by the consumer after construction.
There are two rules for observables with realms: 1. Their state can only be accessed or changed from within their realm. For getters, this is checked by ObservableTracker.getterCalled(). Setters should have a similar check if we want to follow SWT's example, but I haven't implemented that yet. 2. Change events will only be fired within their realm. For example, Java beans observables would have to fire their events using an asyncExec if the underlying PropertyChangeEvent happened on some other thread. I'm sure that there are other solutions to the concurrency problem, but so far I like the realm idea the best because of its simplicity, and because it is a trivial abstraction of SWT's single UI thread model. (One alternative is to lock observables explicitly, which very easily leads to deadlocks.) If you don't want to care about all this threading stuff, you will have to make sure that everything happens on a single thread, so if you use SWT observables at all, everything has to happen on SWT's UI thread. In particular, Java beans observables or the code that creates them would have to know about the UI thread somehow. If realms were optional, it would be non-trivial to use our nifty abstractions like ComputedValue in a multithreaded context. Here are two ideas for how we can (mostly) hide realms in simple cases: 1. (Re-)introduce factories with state, and let the realm be part of that state. I know we already decided against factory instances, and in favour of static factory methods, but it seems worth revisiting the decision in light of the concurrency problem. 2. Support the notion of a "default realm", realized as a thread-local variable. For observables that take a realm argument in their constructor, create an additional constructor without the realm argument, and use the default realm. All you would have to do as a client to let everything happen on the UI thread would be to set the default realm to be the UI thread when you initialize your application. I like (2.) better than (1.) and both better than your optional decorator proposal. What do you think?
I prefer #2 but I need to think about it more especially in the use case of using this in a headless multithreaded environment like a servlet container.
I took a look at the latest code and allowing the consumer to set the realm per thread seems problematic in an environment like Eclipse where views from different authors could be used. View A sets the realm, view B sets the realm, now view A invokes the realm and gets unexpected results because view B implemented a funky realm for the thread. What I've yet to get my head around is is this necessary or possible to implement using a technology that is not SWT? I'm going to try writing a little code against this this weekend, hopefully I'll answer my own question.
We are using thread locals as a kind of "poor man's" parameter passing. You are right that this is brittle, so I would expect that clients always set the realm explicitly (usually to the SWT realm a.k.a. the UI thread), or use a pattern like this: Realm oldRealm = Realm.getDefault(); Realm.setDefault(SWTObservables.getRealm(display)); try { // ... data binding code } finally { Realm.setDefault(oldRealm); } We might want to change setRealm() to return the previous realm to encourage using this pattern. By using thread locals, at least this is not as bad as a global variable. As long as you don't call foreign code from within the try clause, you can be sure that the default realm is set to what you expect it to be, and you are a good citizen if you set it back to its old value. Note that for SWT observables, you don't need to pass the realm. So this is only necessary for model-side observables, or stand-alone ones like WritableValue, WritableList etc. I safer variant of this would be a method like Realm.withDefault(Realm realm, Runnable runnable) that sets the given realm as the default while running the runnable.
Created attachment 52916 [details] thread realm junit Attaching a couple tests and a ThreadRealm implementation to not lose it. A couple of questions: * Should we provide default Realm implementations like ThreadRealm? * Should we provide a way to run a syncExec(...) explicitly? Currently the SWT Realm will run the runnable as an asyncExec(...) if exec(...) is invoked from a thread that is not the UI thread. It seems like when dealing just with an observable this would be necessary. I'll work on fixing the tests and examples this week.
We did this on a framework and it was a big hassle (long hours debugging arcane race conditions, deadlocks, etc) but in the end it was so worth it. It has been a long time since I worked on it so I am a bit rusty, but here is what I can reconstruct: We created a bridge interface lets call IThreadBridge. The interface had 3 methods: public void run(Runnable runnable); public void callbackResult(ICallback callback, Object result); public void callbackError(ICallback callback, Exception exception); ICallback just had two methods: public void result(Object result); public void error(Exception exception); We had some additional metadata in these methods but I left it out for simplicity. The ICallback was written by the consumer, but the IThreadBridge was written by a binding implementor. For example we had two IThreadBridge implementations we packaged with the framework: GenericThreadBridge which passed the call straight through: public void run(Runnable runnable) { runnable.run(); } public void callbackResult(ICallback callback, Object result) { callback.result(result); } public void callbackError(ICallback callback, Exception exception) { callback.error(exception); } and we had a SwingThreadBridge (shudder...) which tossed the call onto the AWT EventQueue. This way we could have one binding implementation, but support multiple threading systems. The extra burden was that for every binding types we had (we only had 5) we had to make sure all calls were made in runnables that were run through the IThreadBridge. This took care of threading from the model to the view. To go the other direction we created our own EventQueue that extended Doug Lea's QueuedExecutor. This managed the thread for our model. To be more flexible we could have just registered two IThreadBridges, one to bridge model to view and the other to bridge view to model. As for passing objects, I have never liked ThreadLocal objects but maybe that was because I never took the time to really understand them. I am much more of a fan of creating immutable objects to achieve thread safety. The two objects that IThreadBridge would pass back and forth were the exception and the result. Exceptions are immutable by design and so you just made sure your result object was also immutable.
(In reply to comment #23) Thanks for writing this up. On first reading, what you have done seems structurally equivalent to the 'Realm' abstraction we came up with. I don't quite understand what the two callback* methods are good for though - when would you use those instead of just run?
(In reply to comment #24) That's a good question... I have been thinking it over for a couple of days and I don't know how to explain what was going on. If I did it over again I would implement it a little more simply, but the basic premise is that threadA invokes a runnable on threadB. ThreadB gets a result and invokes a runnable on threadA. This callback method frees up threadA while threadB is processing. Code running on threadA needs the result (or it wouldn't have started the whole process) and the callback mechanism makes it so threadA doesn't have to block on the data.
(In reply to comment #24) I looked at the Realm patch, but it looks like the Realm is implementing its own workers. First of all that kind of threading work is non-trivial and best left up to a thread package (like the one in 5.0 or Doug Lea's). And second of all, the threading queue might already be set up (as is the case in SWT and Swing).
(In reply to comment #26) > I looked at the Realm patch, but it looks like the Realm is implementing its > own workers. First of all that kind of threading work is non-trivial and > best left up to a thread package (like the one in 5.0 or Doug Lea's). And > second of all, the threading queue might already be set up (as is the case in > SWT and Swing). No, a realm is an abstraction of SWT's UI thread, or if you will, AWT's event queue. Each observable belongs to a single realm, and its setters and getters must be called from that realm. Change events will always be fired from the observable's realm, which means that it is safe to access the observable from listener code (you can be sure that no other thread can call getters or setters concurrently). Just like Display.asyncExec() or EventQueue.invokeLater(), there is a way to have code run within a realm. The concrete SWT realm uses SWT's queue, of course: SWTRealm.asyncExec(Runnable) just passes the runnable to Display.asyncExec(Runnable). In the simplest case, model observables belong to the SWT realm, which just means that you can be sure that their change events are fired on the UI thread. (Even if the underlying PropertyChangeEvents are not.) By abstracting from the concrete SWT types, this can be done in code that has no dependency on SWT. I agree that bridging between realms is non-trivial. This is what bindings are for, and there will be bindings for the different kinds of observables (values, lists, sets, maps, trees) so clients wouldn't have to deal with this. Does it make more sense now? You can also look at the code in the branch mentioned earlier, the patch was really just a first cut.
Created attachment 53260 [details] patch that updates tests to use realms Boris, here are the changes to get the test project to use realms and the latest APIs. I removed the Mocks code and replaced the few tests that used this to simplify things. Also there are still build errors for the following: * CompositeTable binding code * Viewers I rearranged the test suite classes to list all tests and have commented out the ones that don't compile. These are now arranged by package to allow for simpler navigation of the class. Also in the patch my formatter is apparently different than the previous one used so there are more changes than should be. If there's a standard formatter that I should be using let me know and I can recreate the patch with the appropriate format. I'll start on the examples project tomorrow.
Patch released to the Bug116920_investigation branch. (I branched the examples and tests plugin as well.) Brad: Thanks a lot - the tests are green again!
Released ThreadRealmTests, too.
Created attachment 53272 [details] examples patch This patch gets the examples project in line and also fixes a few more of the tests as well. The 002 and 003 snippets run but they are effected by bug 147128. There are still a couple of compile errors but not many.
(In reply to comment #31) > Created an attachment (id=53272) [edit] > examples patch Patch released to the branch.
Merged the changes on the branch back to HEAD.
Just stumbled upon this bug via Brad's blog. I like concurrency problems and I was looking over the Realm implementation. I just have one concern. It looks like the abstract impl of Realm doesn't have a consistent Exception handling strategy. There are two main entry points into Realm, asyncExec() and exec(). In exec(), if you are on the isCurrent() thread, the runnable is executed directly. If an exception happens here, it will spill out of the exec() method into the caller. If you are isCurrent(), it will async() the runnable. By it's vary nature, there can be no exception handling since it's executed on a different thread. It's nice that since you're dealing with Runnables they can't return a value. However, it might be important to know that the runnable actually did execute successfully. My suggestion is that you add the ability to register an "Exception handler" with the Realm. To be consistent, it should apply to both sync() and async() versions of execution. This will allow the caller to handle exceptions appropriately - at a minimum, they could log it. This could come in handy for the impl of DisplayRealm. So if a runtime exception happens during the runnable - it would just say "Unhandled exception occured while processing event loop" or whatever that message is.
Good point Min, thanks for your comments! I filed bug 164343 for this.
http://safari.awprofessional.com/0321349601?ns=2722
Verified that my changes are in I20061213-0800.
