Refactor that `call_api` function in the `Client` (controller) object

[heartsucker]

Right now calling the API from the controller is done by

def call_api(self, function, callback, timeout, *args, current_object=None, **kwargs):
    ...

The timeout function takes no arguments, the callback takes one (result) which is either and exception or whatever the API call returns (Reply, List[Source], etc.).

If want to send a message and then on timeout emit a signal self.reply_failure(reply_uuid) so that the UI element can change the related elements to signify failure, I need to know in the callback what the Reply's uuid is. I can do this:

self.call_api(
    self.api.reply_source,
    self.on_reply,
    functools.partial(self.on_reply_timeout, reply.uuid), # <----- partial
    ... # moar args
)

This would lead to us using partial function application everywhere, which is annoying af because it also means in the tests we can't use checks like mock_thingy.assert_called_once_with(...) because of this:

>>> partial(foo, 123) == partial(foo, 123)
False

The proposal would be something like this:

def call_api(self, function, success_callback, failure_callback, *args, **kwargs):
   ...

Where the failure callback is any failure, not just timeouts. Additionally, both callbacks will be passed *nargs and **kwargs to allow them to make additional decisions about what to do on success/failure.

I do wonder if we might end up wanting even more control in the future over behavior or want additional args available to the callbacks. I can't think of a case right now, but it might be worth removing this all together and having a pattern like

def reply_to_source(self, source_uuid: str, reply_uuid: str, message: str) -> None:
    def func():
        try:
            reply_obj = self.api.reply_source(source_uuid, message, reply_uuid)
        except TimeoutError:
            # foo
        except NotFound:
            # bar
        else:
            # baz

    thread = QThread()
    thread.started.connect(func)
    thread.run()    

I generally feel that we might have abstracted the call_api functionality into a single function too early.

[heartsucker]

I also wonder if this is supposed to be done with signals/slots. So for each API action, we have success/failure slots than get pinged when something breaks. I don't really know when one is supposed to use signals/slots over direct function calls, however.

[kushaldas]

I don't really know when one is supposed to use signals/slots over direct function calls, however.

Signals will help to notify every widget which is supposed to know something. We can use signal/slot mechanism too. Otherwise, I think this can be a good option.

[redshiftzero]

Hmm, if the reason for this change is to know which widget to modify to indicate failure after a timeout or other kind of error applying the change to the server, can't we already do that with the existing logic via current_object? (I think I implemented that during one of the download-related PRs to handle the kind of situation you are describing, where we need to know the id of the submission in the callbacks). Let me know if I misunderstood!

[heartsucker]

Ah right so we could do that, but I guess this still feels like indirection. We have to set the current_object in call_api and then make sure that somewhere else in another function we're passing in the right thing instead of just using it directly in a single large block. It also means if we want to have some kinda finally actions like "no matter what happens on this API call, do X", we have to add that to both call backs.

I also realized that my proposal to just pass a single func in the latter part of the above wouldn't work because the timer needs to wrap the API call, not the whole handler. 😐

[heartsucker]

I thought this would be easy to change, but I keep getting this error which force aborts the application:

QThread: Destroyed while thread is still running

I might just try to restructure the error handling to use more or less the same logic, but at least split it into just success/failure, and not result/timeout.

[heartsucker]

In our current logic, we have QTimers for the call_api methods, but timers don't abort the thread, and even if they did, they calling stop (not terminate, which is dangerous) might not stop the thread. So we are handling a timeout like it's actually an error when the thread might keep chugging along and eventually complete. If we want to handle timeouts for API calls, this needs to be done at the client/sdk/requests level. Unless I'm misunderstanding something?

[sssoleileraaa]

Closing this issues since a lot has changed/been addressed since this issue was opened. For instance, we don't have QTimers for the call_api methods, we only call the call_api function referred to in this issue for api.authenticate and api.logout requests, and we show_login_error when the response is etiher a RequestTimeoutError or ServerConnectionError. Overall, I don't see how the thread might keep working and eventually complete the request since we do get timeout and connection errors from the sdk.