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 <tim.w...@paremus.com> 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<Node>) which should not ever be part of a sensible API contract. > This method could return a Set<Node> (or a Promise<Set<Node>>) 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<Node> handleNodeAddition(Object element) { > > Promise<Node> 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<Set<Node>> doNodeAdditions(List<Object> elements) { > > List<Promise<Node>> promises = new ArrayList<>(elements.size()); > > Promise<Node> 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<Node> 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<Node> 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<Promise<Set<Node>>> addedNodes) { > Promises.resolved(diagram.getNode(element)) > .then(existingNode -> { > Node node = existingNode.getValue(); > Promise<Node> nodePromise; > > if (node == null) { > // Using an AtomicReference so the Promise chain can be > updated > Promise<Set<Node>> 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