Unsupported concurrent calls, throw exception or log a warning?

Question

Context

Let's say I have a navigation service that allows me to navigate to a page. The Navigate method is async because an animation (about 250ms) is involved.

public interface INavigator
{       
    Task<IViewModel> Navigate(Type viewModelType);
}

Problematic use-case: Concurrent calls

Now imagine that have a bottom navigation bar with multiple buttons, each allowing navigation to a specific page. It's very easy tap 2 or more buttons quickly enough to trigger multiple calls to the Navigate method.

I would like my service to discard concurrent calls, allowing only the first one to execute.

Current solution

My current solution is implemented by tracking the state of the navigator. (i.e. Is it already processing a navigation?) When a concurrent call is detected, the method simply logs a warning and returns null. This works fine.

Let's call that Solution 1 for future discussions.

Question

In the context of a class not supporting concurrent calls, what's the best error design?

1. Return a default value? (current implementation, where I also log a warning)
2. Throw an exception? (similarly to what database could do)
3. Have the method return a boolean using a try-out pattern?
4. Have the method return a boolean using a wrapper object?

My thoughts so far

• All solutions involve some handling from the calling code (catch an exception, null check, or bool check).
• Solution 1 can hide failures when the return value isn't used. You don't see that anything went wrong unless you look at logs.
• Solution 2 can seem loud as exceptions involve stopping the execution and capturing the stack trace. Can this cause performance issues? Should I even bother about that?
• Solutions 3 and 4 make the method signature heavier.
• It seems that part of the problem is figuring out whether this use-case is an edge-case.
• I'm asking the question because solution 2 might seem like a better approach because it would be clearer (more explicit).

Answer 1

You should look at it from a user's / caller's point of view. And I doubt that your approach "I would like my service to discard concurrent calls, allowing only the first one to execute" fits that well.

If I understand correctly, calling Navigate() is expected to show the requested view (after playing some animation). And, given enough delay, a second Navigate() call replaces the results of the first call, correct?

So, the call translates to:

I'm no longer interested in what I've navigated to before, now I want to see the newly-selected view.

Rejecting such a request, just because it was issued "too early", is hard to explain to your callers, and makes life difficult for them (detect the problem, wait some amount of time, re-issue the call).

Solution ideas:

• Abort any currenty running Navigate() task, if that's possible, and start the new one.
• Have a queue of Navigate() tasks, and execute them in order of arrival.
• Allow for one executing and one pending Navigate() task. When a new request arrives, have it either become the executing one (if that is available), or override the pending one (thus discarding any intermediate, not-yet-executing requests).

Answer 2

You should consider internal behavior and UX separately.

Internally, an attempt to start a second navigation while there is an ongoing one should be handled just like you would handle other invalid navigation requests, such as navigating to a place that's impossible to reach, or current unavailability of the GPS signal. How you communicate this to the caller is a matter of programming language conventions, most languages use exceptions, but result objects that can contain either a successful result or an error are also common. Just don't sweep exceptions under the rug by silently ignoring (unless your action is idempotent and the exception would only tell the caller that it's already done) or by conflating the possible error conditions into a simple boolean result.

How you handle such exceptions in the UI is a separate issue. You may flash the button, pop up an error dialog, beep loudly, whatever helps the user to realize that they tried something that the system could not perform.

Answer 3

Avoid all errors and exceptions when the code can successfully handle the situation.

In your navigator example the code can easily handle the concurrent actions by discarding, overriding, or queuing the requests and still deliver the expected behaviour. It can even write to the log if required, or raise an event if the concurrent actions need to be exposed.

This is far better than throwing an error if the user clicks too fast.

In terms of UI design, if you really want that animation to run, you should indicate to the user that the other buttons are disabled during the animation. ie. grey them out on the first click and reenable when the animation finishes.

OR I would say the best behaviour, would be for subsequent clicks for different pages to override the in progress navigation without lengthening the animation.

Answer 4

It tends to make things simpler when your methods are idemopotent, so I'd recommend the same here. If the same view is requested again just return the original task.

public interface Navigator
{   
    protected Task<IViewModel> _lastResult = null;

    public async Task<IViewModel> Navigate(Type viewModelType)
    {
        if (_lastResult?.GetType()?.GetGenericArguments().FirstOrDefault() == viewModelType) return _lastResult;
        _lastResult = NavigateInternal(viewModelType);
        return _lastResult;
    }
}

This way the caller only has to deal with one logic path, which simplifies the caller's code and reduces the number of unit tests they will need.

Of course, the best solution is to prevent the concurrency in the first place, if possible, e.g. by disabling the navigation controls while navigation is in progress.

Answer 5

I've been mulling this question around in my head for a couple of days. I think the thing that strikes me most, is how awkward it feels for the navigation service to return... anything.

Typically UI programming solves concurrency problems using events or callbacks. Something like switching views should be handled entirely by the navigation service. The navigation service feels like an incomplete abstraction.

Ideally this should be a fire-and-forget service. Callers should not need to do anything more than inform the nav service of the user's request to go elsewhere. The existing view should be told that it is being deactivated, which allows that object to perform necessary cleanup operations. The next view should be told it is being activated allowing it to perform any necessary initialization steps. Depending on how you manage threads, the teardown of one view and startup of the next could be performed concurrently as the UI buttons are animated. In the very least, the navigation service should be the one to orchestrate the process of deactivate --> animate --> activate.

As a consumer of the navigation service, I want to see a void return type. This tells me that I can call the Navigate(Type) method and do nothing more. Now I don't need to care about the next view. Or the current one. I just need to tell the navigation service where the user chose to go. If the user double-clicks, triple clicks, drags the mouse and bumps the button, or just plain wants to screw with the system to see how it reacts, the navigation service fully encapsulates this behavior, which makes it much easier to decide when to switch views and when to discard a duplicate request.

So I guess my answer is 5) none of the above. The navigation service should not return anything, so callers are not forced to handle a return value (or willfully ignore it at their own peril). Communication between components during the transition phase should be handled by events or callbacks, and the navigation service should orchestrate the whole thing.

Answer 6

I don't think there is a one-size-fits-all answer for this. If there is the risk of data corruption - as in one call may make a bad decision or return a bad value because the other call was ongoing, then, by all means, throw an exception. You may also try to make multiple attempts at the call that was flagged as "dangerous", but that depends on your design.

Leaving the UI at an invalid state is also pretty bad, so managing the state and preventing the possible concurrent calls is a good approach. Wrapping the UI in a state manager that prevents it state to become invalid or inconsistent is also an option in this case.

Most of this design can be implemented outside the method, which, overall, is a good idea - the method shouldn't be too concerned with anything that's not directly related to what it's trying to do and it's not well positioned to manage outer context state validness.