Re: [osgi-dev] Need advice on design pattern using Promises
This helps in some ways, mainly clarifying the difference between
onSuccess() and thenAccept().
Problem is I forgot to indicate I was looking for a solution that works
with a method that does already have a return value; I'm not sure why I
didn't think to use such an example. If you can advise on a case which
already has a return value, I would much appreciate it.
In any case, the solution you gave is roughly what I'd have done if I
didn't keep the original pattern, so I'm glad to know I'm on the right
track with this refactor.
Many thanks!
-Olivier
On Fri, Oct 5, 2018 at 4:33 AM Tim Ward wrote:
> I will start by saying that the original is a fundamentally bad API
> design. It exposes side effects of the method (namely modifying the passed
> in Set) which should not ever be part of a sensible API contract.
> This method could return a Set (or a Promise>) indicating
> what it did, but actually that doesn’t seem to be needed - there’s really
> only one node to return each time. As a result it’s much cleaner and
> simpler to do the set addition and gathering outside this method.
>
> // Using a PromiseFactory is better as it gives more
> // control of threading. You’re already using 1.1
> PromiseFactory pf = new PromiseFactory();
>
> public Promise handleNodeAddition(Object element) {
>
> Promise p;
>
> Node node = diagram.getNode(element);
> if (node == null) {
> p = actionProvider.createNode(element, diagram);
> addedNodes.add(node);
> } else {
> p = pf.resolved(node);
> }
>
> // Using thenAccept means that you return a promise which resolves
> // *after* the synchronize. If you use onSuccess then the returned
> // promise will resolve *before* the synchronize and you may not
> // see the result of the synchronize in some of your other callbacks
>
> return p.thenAccept(actionProvider::synchronize);
> }
>
> The set gathering should then be done elsewhere, and without side-effects.
>
>
> public Promise> doNodeAdditions(List elements) {
>
> List> promises = new ArrayList<>(elements.size());
>
> Promise previous = pf.resolved(null);
>
> for(Object o : elements) {
> previous = previous.flatMap(x -> handleNodeAddition(o));
> promises.add(previous);
> }
>
> return pf.all(promises)
> .map(HashSet::new);
>
> // You could also use this as the promises are all in a chain
> // return previous.map(x -> promises.stream()
> // .map(Promise::getValue)
> // .collect(Collectors.toSet()));
> }
>
> I hope that this helps
>
> Best Regards,
>
> Tim
>
> On 5 Oct 2018, at 00:28, Olivier Labrosse via osgi-dev <
> osgi-dev@mail.osgi.org> wrote:
>
> Hi,
>
> I'm dealing with code refactoring from a synchronous system to an
> asynchronous one, using OSGi Promises. One pattern we have goes as follows:
>
> public void handleNodeAddition(Object element, Set addedNodes) {
> Node node = diagram.getNode(element);
> if (node == null) {
> node = actionProvider.createNode(element, diagram);
> addedNodes.add(node);
> }
> actionProvider.synchronize(node);
> }
>
> The problem I'm facing is that *actionProvider.createNode()* now returns
> a Promise due to asynchronous execution. This means we can no longer
> just add nodes to the Set, but not only this, we have to make sure that
> each createNode() call from this thread happens after the previous one is
> resolved.
>
> Would there be a best practice for this kind of process? If I were to
> keep the pattern as-is but implement support for asynchronous node
> creation, here's how I would do it:
>
> public void handleNodeAddition(Object element,
> AtomicReference>> addedNodes) {
> Promises.resolved(diagram.getNode(element))
> .then(existingNode -> {
> Node node = existingNode.getValue();
> Promise nodePromise;
>
> if (node == null) {
> // Using an AtomicReference so the Promise chain can be
> updated
> Promise> addedNodesPromise =
> addedNodesPromiseRef.get();
>
> nodePromise = addedNodesPromise // wait for previous node to
> be added
> .then(previousNodeAdded ->
> actionProvider.createNode(element, diagram))
> .onSuccess(createdNode ->
> createdNode.setLocation(location));
>
> addedNodesPromiseRef.set(createNodePromise
> .then(createdNode -> {
> addedNodesPromise.getValue().add(createdNode.getValue());
> return addedNodesPromise; // still holds the Set
> })
> );
> }
> else {
> nodePromise = Promises.resolved(node);
> }
>
> return nodePromise;
> })
> .onSuccess(nodeToSync -> actionProvider.synchronize(nodeToSync));
> }
>
> Any and all advice is much appreciated, thank you!
>
>
Re: [osgi-dev] Need advice on design pattern using Promises
I will start by saying that the original is a fundamentally bad API design. It
exposes side effects of the method (namely modifying the passed in Set)
which should not ever be part of a sensible API contract. This method could
return a Set (or a Promise>) indicating what it did, but
actually that doesn’t seem to be needed - there’s really only one node to
return each time. As a result it’s much cleaner and simpler to do the set
addition and gathering outside this method.
// Using a PromiseFactory is better as it gives more
// control of threading. You’re already using 1.1
PromiseFactory pf = new PromiseFactory();
public Promise handleNodeAddition(Object element) {
Promise p;
Node node = diagram.getNode(element);
if (node == null) {
p = actionProvider.createNode(element, diagram);
addedNodes.add(node);
} else {
p = pf.resolved(node);
}
// Using thenAccept means that you return a promise which resolves
// *after* the synchronize. If you use onSuccess then the returned
// promise will resolve *before* the synchronize and you may not
// see the result of the synchronize in some of your other callbacks
return p.thenAccept(actionProvider::synchronize);
}
The set gathering should then be done elsewhere, and without side-effects.
public Promise> doNodeAdditions(List elements) {
List> promises = new ArrayList<>(elements.size());
Promise previous = pf.resolved(null);
for(Object o : elements) {
previous = previous.flatMap(x -> handleNodeAddition(o));
promises.add(previous);
}
return pf.all(promises)
.map(HashSet::new);
// You could also use this as the promises are all in a chain
// return previous.map(x -> promises.stream()
// .map(Promise::getValue)
// .collect(Collectors.toSet()));
}
I hope that this helps
Best Regards,
Tim
> On 5 Oct 2018, at 00:28, Olivier Labrosse via osgi-dev
> wrote:
>
> Hi,
>
> I'm dealing with code refactoring from a synchronous system to an
> asynchronous one, using OSGi Promises. One pattern we have goes as follows:
>
> public void handleNodeAddition(Object element, Set addedNodes) {
> Node node = diagram.getNode(element);
> if (node == null) {
> node = actionProvider.createNode(element, diagram);
> addedNodes.add(node);
> }
> actionProvider.synchronize(node);
> }
>
> The problem I'm facing is that actionProvider.createNode() now returns a
> Promise due to asynchronous execution. This means we can no longer
> just add nodes to the Set, but not only this, we have to make sure that each
> createNode() call from this thread happens after the previous one is resolved.
>
> Would there be a best practice for this kind of process? If I were to keep
> the pattern as-is but implement support for asynchronous node creation,
> here's how I would do it:
>
> public void handleNodeAddition(Object element,
> AtomicReference>> addedNodes) {
> Promises.resolved(diagram.getNode(element))
> .then(existingNode -> {
> Node node = existingNode.getValue();
> Promise nodePromise;
>
> if (node == null) {
> // Using an AtomicReference so the Promise chain can be updated
> Promise> addedNodesPromise = addedNodesPromiseRef.get();
>
> nodePromise = addedNodesPromise // wait for previous node to be
> added
> .then(previousNodeAdded -> actionProvider.createNode(element,
> diagram))
> .onSuccess(createdNode -> createdNode.setLocation(location));
>
> addedNodesPromiseRef.set(createNodePromise
> .then(createdNode -> {
> addedNodesPromise.getValue().add(createdNode.getValue());
> return addedNodesPromise; // still holds the Set
> })
> );
> }
> else {
> nodePromise = Promises.resolved(node);
> }
>
> return nodePromise;
> })
> .onSuccess(nodeToSync -> actionProvider.synchronize(nodeToSync));
> }
>
> Any and all advice is much appreciated, thank you!
>
> -Olivier Labrosse
> ___
> OSGi Developer Mail List
> osgi-dev@mail.osgi.org
> https://mail.osgi.org/mailman/listinfo/osgi-dev
___
OSGi Developer Mail List
osgi-dev@mail.osgi.org
https://mail.osgi.org/mailman/listinfo/osgi-dev
[osgi-dev] Need advice on design pattern using Promises
Hi,
I'm dealing with code refactoring from a synchronous system to an
asynchronous one, using OSGi Promises. One pattern we have goes as follows:
public void handleNodeAddition(Object element, Set addedNodes) {
Node node = diagram.getNode(element);
if (node == null) {
node = actionProvider.createNode(element, diagram);
addedNodes.add(node);
}
actionProvider.synchronize(node);
}
The problem I'm facing is that *actionProvider.createNode()* now returns a
Promise due to asynchronous execution. This means we can no longer
just add nodes to the Set, but not only this, we have to make sure that
each createNode() call from this thread happens after the previous one is
resolved.
Would there be a best practice for this kind of process? If I were to keep
the pattern as-is but implement support for asynchronous node creation,
here's how I would do it:
public void handleNodeAddition(Object element,
AtomicReference>> addedNodes) {
Promises.resolved(diagram.getNode(element))
.then(existingNode -> {
Node node = existingNode.getValue();
Promise nodePromise;
if (node == null) {
// Using an AtomicReference so the Promise chain can be updated
Promise> addedNodesPromise =
addedNodesPromiseRef.get();
nodePromise = addedNodesPromise // wait for previous node to be
added
.then(previousNodeAdded ->
actionProvider.createNode(element, diagram))
.onSuccess(createdNode ->
createdNode.setLocation(location));
addedNodesPromiseRef.set(createNodePromise
.then(createdNode -> {
addedNodesPromise.getValue().add(createdNode.getValue());
return addedNodesPromise; // still holds the Set
})
);
}
else {
nodePromise = Promises.resolved(node);
}
return nodePromise;
})
.onSuccess(nodeToSync -> actionProvider.synchronize(nodeToSync));
}
Any and all advice is much appreciated, thank you!
-Olivier Labrosse
___
OSGi Developer Mail List
osgi-dev@mail.osgi.org
https://mail.osgi.org/mailman/listinfo/osgi-dev
