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!
>
>
