5

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).
8
  • 3
    I'm not sure what your goal is with this. Discarding the call is the expected behavior. Why do you need to log anything at all? You mention "hiding failures"; perhaps you can elaborate on that. There's a risk here of polluting you logs with uninteresting information that could be a red-herring later.
    – JimmyJames
    Apr 22 at 17:20
  • 1
    I'm sorry if this seems trivial, but a simplistic solution 3 could be to lock the button clicks while the async method hasn't completed. If that is not possible, please disregard...
    – Silvio
    Apr 22 at 17:42
  • @JimmyJames I know I want to discard concurrent calls, but I'm wondering how. Should the method just do nothing? Should it log a warning (or another log level)? Should it throw an exception?
    – Batesias
    Apr 22 at 17:42
  • 4
    Right, that was the context of the question. What is the point of logging this if it's an expected result? Who is going to look at it? What are they going to do about it? It just seems like noise to me.
    – JimmyJames
    Apr 22 at 20:04
  • 1
    @JimmyJames I like when things are clear. When you call the Navigate method but you don't actually navigate, you might wonder why. Having explicit feedback that "the navigation request was discarded because another one is processing" seems like a reasonable behavior. Perhaps the log level should be lower than warning though because, like you said, there isn't much to do about it.
    – Batesias
    Apr 22 at 20:18

6 Answers 6

2

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).
3
  • No, the second call does not replace the previous one. The first call wins. The second call is ignored.
    – Batesias
    Apr 25 at 17:10
  • 2
    @Batesias Just to understand the use case correctly. Your callers want the second call to be ignored if the first hasn't completed, but to succeed if the first one has completed? My impression is that the way you implement it might not match the intentions of your users. Apr 26 at 10:00
  • That's correct. Historically, concurrent calls behaved like a queue, resulting in double navigations. This was problematic because users would accidentally double tap the same button or two different buttons and open 2 pages instead of 1. (We're on phones here, so it's more frequent on touch screens than with mouse and keyboard.) We decided to throttle those calls by discarding the concurrent ones.
    – Batesias
    Apr 26 at 12:17
1

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.

1

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.

1
  • Ok so not solution 2. What about 1, 3, and 4; is there one that should be preferred?
    – Batesias
    Apr 25 at 13:01
1

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.

1
  • I can apply this solution when the type is the same. However this is not always the case in my scenario. Concurrent calls currently target multiple types.
    – Batesias
    Apr 24 at 12:28
1

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.

7
  • I concur with this. Having navigation return a value implies that the caller is somehow responsible. This is an inversion of responsibility, and not in a good way. The navigator is responsible for the navigation, but the button on the fourth sub module as it called the navigator is responsible for whether or not the fifth pages report model can be created. It doesn't mesh well.
    – Kain0_0
    Apr 26 at 10:41
  • Interesting! I need to point out that this abstraction is indeed not dependent on any view/UI class. It lives in the layer just above which is why it's using ViewModels and not views. This allows us to have an implementation for automated tests allowing us to run our app virtually (without any views) from a Tests project. We obviously also have an implementation that manipulates UI.
    – Batesias
    Apr 26 at 13:20
  • You can still write automated tests for event driven code. Just define a simple interface that components must satisfy in order for the navigation service to interact with them. Pass mocks or stubs during testing. I would even recommend defining an interface for the animation as well. You can pass a mock during testing and perform extremely fine-grained tests on the navigation service to see how it handles edge cases. The mock animator could be controlled completely by the test. (1/2) Apr 26 at 14:34
  • (2/2) and the tests would not need to by async either. The mock animator would have methods to "start" the animation and trigger the "end" of the animation as well. The test code would appear procedural, but the application could still behave asynchronously during regular use. Apr 26 at 14:36
  • Right, I was just explaining that by design the interface is abstract enough to work with or without views. The full interface actually has a State property and a StateChanged event. I could indeed change the signature of Navigate to remove the return value and still be able await the result of the navigation via the State and StateChanged members. However, it seems like doing so would delay the concurrency issues to the code running after the navigation.
    – Batesias
    Apr 26 at 15:46
1

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.