-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Possible inconsistency in BufferMapper exposed properties #28
Comments
What is the current behaviour, if an observer throws? |
Indeed, the clearTrigger() is the same story as the Sink discussion ;-) ... I totally forgot this ;-) |
The current behavior is to not affect other observers or the producer - the exception be caught internally (in DispatchingObservable), wrapped into an UpdateDeliveryException, and sent to the global uncaught exception handler (typically something that would simply log the exception). Basically observers are not expected to throw. |
Lets imagine you do the clearTrigger() by exposing a property:
And you have somewhere another
which shall (on any update) clear your buffer. Ok, then one could of course (assuming we would have implemented binding, which we do not have yet, or?) do something like:
However, what prevents you from then doing
... and suddenly, any set() on the clear() property would also notify all other subscribers of triggerInput... My main point is: The clearTrigger() shall only observe. It shall not be observable.... With the current implementation it is simply a:
|
I think the behaviour you describe above would be ok ... |
Ok, what one can do of course always is, just expose a method And then subscribe using a lambda ...
... in the end, you do not listen to exceptions .... |
For the maxValue() I agree, this is not clean. Your proposal sounds fine... Could the simple property, potentially just have an additional constructor method (a validator) ...in the simplest case a Consumer that could throw? |
Indeed I would say if you need this behavior, you should build an observer yourself - in the simplest case something like In general for setting things, I think a wrapperProperty() is the right way to do it. I don't like the fact that the actual change is triggered by a subscription to the property - apart from the problem that there is no way anymore to refuse updates at this point, there is also no guarantee that the change has actually happened at the time the other observers are notified. |
For my binding post above, let me reformulate something: Currently in our concepts we can have (at least in mind) something like
where b can prevent bidirectional binding (by being an ObservableValue). However, a has no way to do so (it always has to be a property ...) .... I think I have to rethink all this quietly ;-) ... Seems that all this mixing of state/set/bidirectionality is too much for my brain ... |
Together with Andrea, we found out that the maxSize() property exposed by BufferMapper can be inconsistent if a negative value is set.
In this case, the internal subscriber in BufferMapper will throw, but it will only get the update as soon as any other subscriber gets it - so it cannot refuse it at that point, the value of the property will have been set already.
In such cases, we should use probably use WrapperProperty with an own implementation of set(), that validates, possibly throws, and only then notifies subscriber of a change (if any).
Also, after some more thinking I would be in favor of removing clearTrigger(). In general, I would never expose Observers (only observables and possibly properties) for the same reason I was against introducing "processors" or similar:
It takes the control over the data flow away from the service provider, which then e.g. can not refuse updates. In this case it is probably fine as clear() can not throw and does not care about the data, but if it was some value and not all values were accepted, it would actually create exactly the same problem as with the SimpleProperty for maxSize() above.
The text was updated successfully, but these errors were encountered: