Skip to content
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

How best to handle asynchronous commands? #39

Open
rlrh opened this issue Feb 25, 2019 · 10 comments
Open

How best to handle asynchronous commands? #39

rlrh opened this issue Feb 25, 2019 · 10 comments
Labels
question Further information is requested

Comments

@rlrh
Copy link

rlrh commented Feb 25, 2019

I don't know if I can ask about this, but if it's possible, can I get pointers on how best to handle asynchronous Commands?

For example, there might be a Command that involves fetching data from the Internet, but it should return first to keep the UI responsive while a background task does the fetching and updates the Model. However, this isn't possible currently because the LogicManager only saves the AddressBook to disk upon command execution, and also the ResultDisplay is only updated upon command execution.

There must be many possible ways to go about it, but I'm asking because it looks like there was an EventsCenter in the past that would seemingly make all these much easier, but has been removed now. I wonder if this removal is for practical or pedagogical reasons? Would it be better to spend effort reimplementing something like EventsCenter to prevent future headache or try to work around the limitations of the current design? Thanks.

@okkhoy
Copy link
Collaborator

okkhoy commented Feb 25, 2019

EventsCenter was removed because it was over-engineered in place to make AB-4 event-driven.
Outright I don't have an answer to "how to best handle async commands".
If it is necessary for your project, you may want to implement something simple yourself. I can check if AB developers can give you any pointers.

@okkhoy okkhoy added the question Further information is requested label Feb 25, 2019
@pyokagan
Copy link

@rlrh

For example, there might be a Command that involves fetching data from the Internet, but it should return first to keep the UI responsive while a background task does the fetching and updates the Model.

How are you planning to implement the background task in the first place? Which relevant async APIs/libraries are you trying to use?

Ultimately no matter how you implement it (EventBus or some other method), just be careful to ensure that any interaction with the Model/LogicManager/Ui should only be done on the JavaFX Application Thread.

@pyokagan
Copy link

For students who want to pursue this: Just be very aware of threading issues, since Java code can create their own threads at will, as opposed to something like NodeJS which doesn't expose threads to Javascript code and has a single "blessed" event loop that is hidden. I'd highly recommend students who have no prior experience with threading-related issues not to attempt it, especially given the short duration of your project.

@rlrh
Copy link
Author

rlrh commented Feb 25, 2019

@rlrh

For example, there might be a Command that involves fetching data from the Internet, but it should return first to keep the UI responsive while a background task does the fetching and updates the Model.

How are you planning to implement the background task in the first place? Which relevant async APIs/libraries are you trying to use?

Ultimately no matter how you implement it (EventBus or some other method), just be careful to ensure that any interaction with the Model/LogicManager/Ui should only be done on the JavaFX Application Thread.

@pyokagan Thanks for your advice! For now I'm planning to use what's provided in javafx.concurrent as described here, since it's supposed to work well with JavaFX.

After jerry-rigging AB4, I've managed to do some long-running stuff in a Task on a separate Thread, updating the Model with Platform.runLater. The UI remains responsive while the long-running stuff is done in the background. The problem is that the CommandResult is returned immediately before the Model is updated, and as mentioned above, the LogicManager only saves the AddressBook on command execution, and the ResultDisplay only updates on command execution. I'm potentially confusing multiple issues in this thread, but I think they're related.

Jerry-rigged ExperimentalAdd command relevant code:

    public CommandResult execute(Model model, CommandHistory history) {
        Task<Void> task = new Task<>() { // background task
            @Override
            public Void call() throws Exception {
                Thread.sleep(5000); // some long-running task
                Platform.runLater(() -> {
                    model.addPerson(toAdd);
                    model.commitAddressBook();
                });
                return null;
            }
        };
        new Thread(task).start();
        return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd)); // does not wait for background task
    }

LogicManager relevant code:

public LogicManager(Model model, Storage storage) {
        ...
        // Set addressBookModified to true whenever the models' address book is modified.
        model.getAddressBook().addListener(observable -> addressBookModified = true);
    }

    @Override
    public CommandResult execute(String commandText) {
        ...
        addressBookModified = false;
        CommandResult commandResult = command.execute(model, history);
        if (addressBookModified) {
                storage.saveAddressBook(model.getAddressBook());
        }
        ...
        return commandResult;
    }
}

MainWindow relevant code:

    private CommandResult executeCommand(String commandText) throws CommandException, ParseException {
        ...
        try {
            CommandResult commandResult = logic.execute(commandText);
            resultDisplay.setFeedbackSuccessToUser(commandResult.getFeedbackToUser());
            return commandResult;
        } catch (CommandException | ParseException e) {
            resultDisplay.setFeedbackErrorToUser(e.getMessage());
            throw e;
        }
        ...
    }

I guess my question is actually what are some good ways to decouple command execution from storage and UI updates that are not too different from the current design. There are so many possible ways to do it, but not solving it properly could make things really messy fast.

@pyokagan
Copy link

@rlrh

I guess my question is actually what are some good ways to decouple command execution from storage and UI updates that are not too different from the current design. There are so many possible ways to do it, but not solving it properly could make things really messy fast.

Unfortunately I can't provide a definite recommendation since concurrent programming is out of CS2103's scope, so it simply isn't considered at all in AB-4.

You could try out some designs and let us know how it goes :-)

@pyokagan
Copy link

pyokagan commented Feb 25, 2019

@rlrh

In case you actually wanted confirmation:

The UI remains responsive while the long-running stuff is done in the background. The problem is that the CommandResult is returned immediately before the Model is updated, and as mentioned above, the LogicManager only saves the AddressBook on command execution, and the ResultDisplay only updates on command execution. I'm potentially confusing multiple issues in this thread, but I think they're related.

That sounds about right to me. If a function becomes async, all callers would also need to be rewritten to be async as well.

@rlrh
Copy link
Author

rlrh commented Feb 25, 2019

I did some digging around and Guava's EventBus seems like an uncomplicated way to solve these problems. Would it alright to go back in time and take some EventsCenter code as well as the earlier versions of components that used EventsCenter?

@okkhoy
Copy link
Collaborator

okkhoy commented Feb 25, 2019

@rlrh go ahead and do it if interested. we won't stop you from learning or trying things 🙂

However, when it comes to counting beans, I am not too sure "taking EventsCenter code and earlier versions of components" would count as your contribution 🙁
Anything else that you build on top of this would be yours to claim.

@rlrh
Copy link
Author

rlrh commented Feb 25, 2019

@okkhoy Aha yes counting beans xD I just took a look at the issue about removing EventBus and the migration strategy, which gives a clear explanation of the current and previous design. Thank you @okkhoy and @pyokagan , I guess it's on my team to figure out how to proceed now.

@pyokagan
Copy link

Sure, do post about your experiences when you are done :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants