Community
Participate
Working Groups
I think it would be nice to have a direct support for asynchronous validation and conversion on a Binding level during the Binding's update in either direction. Ideally, while waiting for the validation/conversion to be completed, the update's destination observable should not be modified. In addition, the Binding should probably expose some (observable) information which indicates that a validation/conversion is pending (maybe analogous to the staleness feature on an IObservable).
Would you be interested in contributing a patch? http://wiki.eclipse.org/Platform_UI/How_to_Contribute
Hi Boris, actually, I could give it a try but I cannot guarantee anything as I have never contributed to such a project. In any case, I'll be happy to give it some thought and see what I can do.
BTW, I have a conceptual question about introducing support for asynchronous updates: Assume we have an asynchronous conversion from target -> model. If we now have the following statements target.setValue("hello"); // asynchronous update will take some time model.setValue("bye"); // ... // at some point here, the converted target value will be set on the model the model observable will end up having the value "hello" and not "bye" as one would probably expect unless the asynchronous update from target -> model would be canceled ASA the value "bye" is set on the model observable. Am I right in the assumption that the above update from target -> model should be canceled ASA an event occurs (like setting the model value above) which would make the pending update obsolete anyway? Essentially, this would mean that whenever the target's or model's value changes, any potentially running update on the binding would be canceled. Any feedback anyone?
I think your observation about a setValue on the model overriding a pending update from target is correct. This is the behavior for MultiValidator.observeValidatedValue() and SWTObservables.observeDelayedValue(). Regarding your observation comment #3, it is conceivable in the current databinding code for the model and target to go out of sync with each other if both are updated to diverging values around the same time in their respective realms. Shouldn't be too hard to test for. We should look into this for 3.5.
Matthew, thanks for the feedback. It's good to know that similar things have to be considered in other parts of the DB framework. I think it would be nice if the two updates in my example would be correctly serialized as clients will probably expect that, even if the updates are executed asynchronously. In any case, as you say, it shouldn't be too hard to get this right.
Here is a patch for a possible approach of introducing asynchronous validations and conversions for ValueBindings. Let me explain the main ideas of the current implementation (which all in all is rather simple) while also mentioning some of the problems I have encountered so far. * The validators and converters now have a new method isAsync() which expresses whether the validator/converter is intended to be run asynchronously. * The UpdateStrategy class also has an isAsync() method which, by default, delegates to the individual corresponding methods of the validators/converters associated to it even though an UpdateStrategy may by itself explicitly express the intent to be run asynchronously by overwriting this method. * Ultimately, it is up to the Binding implementation to decide whether to honor those requests for asynchronous execution by really implementing the updates asynchronously. * Whenever an asynchronous validation/conversion is pending on a Binding, the staleness state of the Binding's validation status observable is set accordingly. * The WizardPageSupport class tracks the staleness state of the individual validations in order to wait for pending validations by temporarily setting the wizard page's completion state to false. * I have slightly modified the MultiValidator class to let subclasses notify the MultiValidator that a validation is being executed asynchronously. That way, the MultiValidator can set the staleness state of its validation status observable accordingly and, thus, can be treated like an asynchronous binding. This e.g. allows for asynchronous cross field validations. * Situations like the one in my previous comment #3 should be correctly handled by eventually canceling a previous update when a new one comes in. Note that the above right now only works realiably for ValueBindings which is the simplest case (even though probably also the most important one in practice). ListBindings (and SetBindings) are more complicated, especially if you try to avoid the target and model lists getting out of sync. The main reason for that is that in ListBinding updates, you only transfer diff entries which may not apply anymore due to another, still pending asynchronous update in the other direction. In the patch, I'm also providing a basic implementation for asynchronous conversions on ListBindings but it's really merely to show up the problems since the lists can indeed get out of sync and the update may even fail due to outdated diff entries. However, even though the implementation for ListBindings cannot be used at all in practice, it may be a good starting point to play around and try to get this right, too. In the patch, you can also find two simple snippets. One illustrates the actual asynchronous updates for ValueBindings as well as ListBindings, again, the latter only for demonstration purposes. The other shows an example of using asynchronous single and cross field validations in a wizard and how the WizardPageSupport class tracks the pending validations. Of course, the current implementation is rather simplistic since the asynchronous updates of Bindings are executed in isolation, i.e. there is no support for cross-binding dependencies which would be really nice (and probably complex :-)). However, I would be interested in getting some feedback about the current approach and how it could be improved, mainly to avoid having me taking a totally wrong approach. I'd also be happy to take this further if there is any interest from others. Note: I've introduced a new dependency on the org.eclipse.core.jobs plugin, mainly to take advantage of the thread pool provided by the Jobs API. Please just let me know whether this is OK.
Created attachment 105098 [details] patch introducing asynchronous validations/conversions on a ValueBinding (includes some snippets)
Wow, that was a giant patch. I haven't digested everything yet, but I have a couple questions for now: 1. Due to the sheer size of this patch it's going to require extensive IP review. However there are some small gems in this patch that are not related to asynchronous validation/conversion which could be broken off. Could we separate these out into separate patches in separate bug reports? We can always add those bugs as dependencies to this one and it would make the approval process that much easier. Specifically I'm thinking of your WizardPageSupport fix for checking staleness, ControlDecorator, the small fix in UnmodifiableObservableValue, UpdateExecutor/UpdateRunnable, and possibly others I missed. 2. Would it be possible to encapsulate the asynchronous execution into more fine-grained pieces rather than adding isAsync() methods everywhere? I'm thinking: new UpdateValueStrategy() .setAfterGetValidator(new AsyncValidator(asyncExecutor, theValidator)) .setConverter(new AsyncConverter(asyncExecutor, theConverter)) Like I said, I haven't fully digested the problem but I'm wondering if something simpler would be possible that avoids mass changes to the code base. 3. I'm concerned about the general possibility of bindings going out of sync that is possible right now. I think that UpdateExecutor is a good starting point for solving this problem. I'd prefer to solve this first before we approach the asynchronous conversion/validation feature (I have a feeling that solving the former will help raise a simpler solution to the latter).
Matthew, thanks for the immediate feedback and sorry for the somewhat lengthy patch and post above :-). Some inline comments below... > 1. Due to the sheer size of this patch it's going to require extensive IP > review. However there are some small gems in this patch that are not related > to asynchronous validation/conversion which could be broken off. Could we > separate these out into separate patches in separate bug reports? We can > always add those bugs as dependencies to this one and it would make the > approval process that much easier. Specifically I'm thinking of your > WizardPageSupport fix for checking staleness, ControlDecorator, the small fix > in UnmodifiableObservableValue, UpdateExecutor/UpdateRunnable, and possibly > others I missed. Actually, there are some things which can be separated out: * The WizardPageSupport part is totally independent and can be moved out of the patch. * The MultiValidator changes are also independent. * The tiny fix in UnmodifiableObservableValue actually fixes bug #237163 and I merely needed it to get the MultiValidator stuff working so this can also be removed. * Note that the ControlDecorator is merely part of the examples plugin and is currently only used in one of my snippets which visualizes the fact that an asynchronous update is pending. This can obviously be moved out. * The second snippet can be removed (transitively :-)) since it merely illustrates things related to the WizardPageSupport and MultiValidator changes. * I'd suggest that I also remove the part related to the ListBinding since that one is not working yet while concentrating on the ValueBinding part. Shall I adapt the patch accordingly and/or provide separate enhancement requests for the stuff above? > 2. Would it be possible to encapsulate the asynchronous execution into more > fine-grained pieces rather than adding isAsync() methods everywhere? I'm > thinking: > > new UpdateValueStrategy() > .setAfterGetValidator(new AsyncValidator(asyncExecutor, theValidator)) > .setConverter(new AsyncConverter(asyncExecutor, theConverter)) > > Like I said, I haven't fully digested the problem but I'm wondering if > something simpler would be possible that avoids mass changes to the code base. The reason why I did it using the isAsync() methods is that, IMO, the validator/converter should be the one that states that it is intended to be executed asynchronously while the actual asynchronous execution is performed by the Binding. Therefore, you probably need either a method expressing that intent for asynchronous execution, or else, you use a marker interface or the like. The problem with the latter is that the update strategies do not expose the actual validators/converters. That's why I have also introduced the isAsync() method on the UpdateStrategy to make that information accessible to the Binding. However, a possible option would be to only introduce the isAsync() method on the update strategy while using some less invasive marker interfaces (IAsyncValidator/IAsyncConverter) for the validators/converters. That would also work even though marker interfaces are a bit less flexible. Would you prefer something like that? > 3. I'm concerned about the general possibility of bindings going out of sync > that is possible right now. I think that UpdateExecutor is a good starting > point for solving this problem. I'd prefer to solve this first before we > approach the asynchronous conversion/validation feature (I have a feeling that > solving the former will help raise a simpler solution to the latter). Note that with the current patch, a ValueBinding will never get out of sync as long as target and model are in the same realm, even if the binding updates are executed asynchronously. So, essentially, the current implementation IMHO provides the same guarantees as we had before, at least on a binding level. However, as already mentioned, dependencies among different bindings are not considered yet so what indeed can happen in more complex settings is that you end up with different values in your model than you would have had when executing everything synchronously. The UpdateExecutor is actually used to serialize some otherwise asynchronous updates and is currently only used on a binding level to ensure that a single binding does not get out of sync. Eventually, this could be extended to a whole DataBindingContext or the like. Ideally, the user should probably be able to decide on which granularity it wants otherwise asynchronous updates to be serialized, or else, it should be able to explicitly express dependencies among bindings. And that's where things get a bit more complex and interesting :-).
I have an idea for how we could add asynchronous validation/conversion and isolate the changes to ValueBinding and UpdateValueStrategy: 1. In UpdateValueStrategy, add API for specifying whether validation should be done asynchronously. This could take any number of forms: // use Job.schedule() setAfterGetValidator(IValidator validator, boolean async) // use Realm.asyncExec() setAfterGetValidatorRealm(Realm) // use UpdateExecutor.execute() setAfterGetValidator(IValidator validator, UpdateExecutor executor) 2. Introduce a callback interface for handling status: interface StatusHandler { public void handleStatus(IStatus status); } 3. Add methods that shadow the current validation/conversion methods in UpdateValueStrategy but which use a callback instead of returning a value: UpdateValueStrategy.java: public void validateAfterGet(final Object value, final StatusHandler callback) { if (validateAfterConvertRealm == null || validateAfterConvertRealm.isCurrent()) { callback.handleStatus(validateAfterGet(value)); } else { validateAfterConvertRealm.exec(new Runnable() { public void run() { callback.handleStatus(validateAfterGet(value)); } }); } } 3. Change ValueBinding to use the callback methods in place of the current methods. I think this approach could be applied to the other binding classes as well. Thoughts?
Furthermore it might not be a bad idea to make the validation status stale at the start of the binding pipeline.
Created attachment 105180 [details] Alternative MultiValidator patch for asynchronous validation
Note to self: ValidatedObservable(Value|Set|List|Map) should suspend updates while the MultiValidator's validation status observable is stale
Created attachment 105188 [details] MultiValidator asynchronous validation - proof of concept If you run Snippet019 with this patch you'll see the (artificial) delay due to the long-running validation in a separate thread, and how it affects the validation message as well as when the target is copied to model.
Created attachment 105214 [details] First attempt at callback strategy in ValueBinding and UpdateValueStrategy
(In reply to comment #14) > If you run Snippet019 with this patch Sorry, that should have been Snippet021
Matthew, first of all thanks for the alternative patches. It's always good to throw in new ideas. (In reply to comment #14) > Created an attachment (id=105188) [details] > MultiValidator asynchronous validation - proof of concept > > If you run Snippet019 with this patch you'll see the (artificial) delay due to > the long-running validation in a separate thread, and how it affects the > validation message as well as when the target is copied to model. I think that the approach using the callback is not much different from what I was doing with the enterStale()/exitStale(IStatus) methods. However, there is a subtle difference: In your code, you are always setting the validation observable to be stale in the revalidate() method when starting a new validation, even if the validation is being performed synchronously. IMHO, the staleness state should only be set if the validation is being performed asynchronously. In my implementation this is achieved by having clients only use the enterStale()/exitStale(IStatus) methods if in async mode. Otherwise, they simply return the status in the validate() method, as always. That's really just a tiny detail but you would e.g. notice it if you had the WizardPageSupport class listen on the staleness state of a validation and accordingly set the wizard page to be not complete if any validation is pending. In that case, synchronous validations would also lead to the wizard page being set to be not complete which could result in some flickering of the enablement state of the next button. Similar effects could be noticed in other parts of the GUI. BTW, it's nice that you have also adapted the ValidatedObservable(Value|Set|List|Map) to listen on the staleness state, too!
(In reply to comment #10) > I have an idea for how we could add asynchronous validation/conversion and > isolate the changes to ValueBinding and UpdateValueStrategy: > > 1. In UpdateValueStrategy, add API for specifying whether validation should be > done asynchronously. This could take any number of forms: > > // use Job.schedule() > setAfterGetValidator(IValidator validator, boolean async) > // use Realm.asyncExec() > setAfterGetValidatorRealm(Realm) > // use UpdateExecutor.execute() > setAfterGetValidator(IValidator validator, UpdateExecutor executor) Rather than using a Realm I personally would tend to use a specialized class, be it the UpdateExecutor or something else since you may want to support additional operations like the canceling of a pending update or the like which does not apply to a Realm. What I'm not sure about is whether you really need a separate Realm/UpdateExecutor for every validator/converter since in practice I would say that what you want to do is simply execute a validation/conversion asynchronously, not matter in which thread. In addition, if some validator/converter really needs to execute in a specific thread, I think this should be ensured in the validator/converter itself anyway to make this reliable. So maybe it would be simpler and flexible enough to introduce a single Realm/UpdateExecutor per update strategy? > [...] > I think this approach could be applied to the other binding classes as well. > Thoughts? I really think you need something more to get this right for (List|Set)Bindings. As a simple example, consider a setting similar to that of comment #3 but for list bindings: targetList.add("hello"); // asynchronous update will take some time modelList.remove("hello"); // ... // remove gets executed before "hello" is inserted into the modelList // and thus does nothing at all, leaving "hello" in the list. Here you would have to avoid that the remove method takes effect while the update from target to model has not finished but this is not easily possible since only the modelList observable can control this. One possible solution could be to create a wrapper observable around the target/model observable of the binding which only applies changes to the actual observable if no updates are pending. That's basically similar to what the ValidatedObservable(Value|Set|List|Map) do in the MultiValidator context. Another approach for list/set bindings could be to make the corresponding Diffs carry more information about the actual change, i.e. not simply the position of an element's addition/removal but rather the element itself. In addition, operations like clear() should have a dedicated representation in a Diff since if you have e.g. a list containing the two strings "v1" and "v2", it is really different whether you remove the two elements from the list using the removeAll(Collection) method or rather using the clear() method. So maybe (I still have to think this through) having some richer Diffs for lists/sets could allow to implement asynchronous updates without the need for cumbersome wrapper observables as described in the first approach above. Of course, we would have to see whether the existing Diffs could be extended while ensuring backwards compatibitiliy. Those are really just some thoughts so feedback is very welcome.
Matthew, as you had suggested initially, I have created separate bug reports for the things related to the WizardPageSupport (bug #237856) and MultiValidator (bug #237857) classes. So, this bug report could be used for the actual Bindings part only which is certainly the most involved. As I find the time, I will split up my initial patch.
Created attachment 105579 [details] proposed patch for asynchronous ValueBindings (includes some unit tests and a snippet) That's the stripped-down version of the patch which contains the changes related to asynchronous ValueBindings only.
Created attachment 106547 [details] Latest code on asynchronous validation / conversion in ValueBinding using callbacks Ovidio, this patch is still using realms as the method for branching validation and conversion to other threads of execution. I'm not in love with the realms approach and could use some alternate ideas. A couple guidelines: * I do not want to add any dependencies to new plugins. I merely used realms because it is an existing concept in data binding. * I do not want a solution that changes code all over the place. This to me is a hint of a design problem; we should be able to focus this functionality into as tight a space as possible to minimize impact on the rest of the system. I've thought about introducing two new interfaces, IAsyncValidator and IAsyncConverter which would be copies of the existing interfaces, except that these classes would have an additional Callback parameter. Tell me what you think. Also I want to say that it's been a pleasure reading your patches; you've been very thorough down to the last detail, and you seem to "get" our design philosophy. Like Boris, I hope that you will consider joining the project as a committer.
Created attachment 106548 [details] mylyn/context/zip
(In reply to comment #21) > Ovidio, this patch is still using realms as the method for branching validation > and conversion to other threads of execution. I'm not in love with the realms > approach and could use some alternate ideas. A couple guidelines: > > * I do not want to add any dependencies to new plugins. I merely used realms > because it is an existing concept in data binding. > * I do not want a solution that changes code all over the place. This to me is > a hint of a design problem; we should be able to focus this functionality into > as tight a space as possible to minimize impact on the rest of the system. > > I've thought about introducing two new interfaces, IAsyncValidator and > IAsyncConverter which would be copies of the existing interfaces, except that > these classes would have an additional Callback parameter. Tell me what you > think. I agree on both your guidelines and I think that introducing a Callback parameter is a good idea since it allows to move the actual branching to a different thread out of the framework, thus removing the dependency on a concrete API (Realm, Job) while still providing an easy-to-implement pattern to clients. In addition, I do not see any particular problem with the Callback approach to implement the same functionality provided by my patch so I think we should definitely follow that path. > Also I want to say that it's been a pleasure reading your patches; you've been > very thorough down to the last detail, and you seem to "get" our design > philosophy. Like Boris, I hope that you will consider joining the project as a > committer. Thanks, I'm really happy to hear that. And many thanks also for having looked at the code! However, unfortunately I have just found a bug in my patch which could be a more fundamental problem of the current approach. The point is that I was claiming that the current implementation guarantees that a series of updates on a single Binding always leads to the same result as if executed synchronously. This is true for updates which actually reach the Binding but there are cases in which no update is triggered. Here is an example in which this could happen: target.setValue("v1"); // update completes and writes "v1" to model target.setValue("v2"); // update takes some time to complete model.setValue("v1"); // model still contains "v1" as update above has not // completed yet => no value change event is fired! // "v2" is written to model as the update above has not been canceled! Obviously, in the synchronous case, the target and model would end up containing the value "v1" but in the asynchronous case this is not guaranteed! So probably, we would need some wrapper observable around the target/model which knows about already pending updates and which either delays setting the observable's value or else it always fires a value change event if there are pending updates. However, I don't like the idea of requiring some wrapper observable as this imposes an additional burden on clients. Any thoughts on that?
Any chance something like this will be targeted for 3.5?
(In reply to comment #24) > Any chance something like this will be targeted for 3.5? It would have to be done soon since M6 is the API freeze. Looking at the last couple of comments, it looks like there are still some open questions. Ovidio, are you still around? What would have to be done to make this real?
I'll see what I can do before M6.
(In reply to comment #25) > Ovidio, are you still around? What would have to be done to make this real? Boris, sorry for my very long absence, it's actually a bit sad. Regarding this bug, the only problem seems to be the one I've mentioned in comment #23. In order to solve the problem using the approach taken so far we would need to somehow notify the Binding whenever a value is set on the observable. This could be done having special (wrapper) observables which *always* fire a change event when a value is set on them or else we could introduce some additional means directly in the method AbstractObservableValue#setValue which could trigger some special event representing the fact of a value having been set (which might itself not lead to an actual change event). Otherwise, I guess we would have to take some conceptually different approach to the problem.
We're out of time for 3.5--there are still too many open-ended questions and there's not enough time left to properly tease out the answers to them. Anybody interested please ping me at the beginning of the next release cycle (after the API thaw) and we'll start over on this.
