CancelUpload only works if the upload is already paused

[joseffilzmaier]

Hello everyone!

Using the ImageManager i tried to cancel a upload using its cancelUpload() method, but i still got callbacks after doing so. I inspected the code and saw that the upload is only fully cancelled if the file transfer was paused to begin with:

    public synchronized void cancelUpload() {
        if (mUploadState == STATE_NONE) {
            LOG.debug("Image upload is not in progress");
        } else if (mUploadState == STATE_PAUSED) {
            LOG.debug("Upload canceled!");
            resetUpload();
            mUploadCallback.onUploadCanceled();
            mUploadCallback = null;
        }
        mUploadState = STATE_NONE;
    }

My question: Is this the intended design? - Shouldn't the upload be cancelled also if the upload is currently in progress and not paused? Like so:

    public synchronized void cancelUpload() {
        if (mUploadState == STATE_NONE) {
            LOG.debug("Image upload is not in progress");
        } else if (mUploadState == STATE_PAUSED || mUploadState == STATE_UPLOADING) {
            LOG.debug("Upload canceled!");
            resetUpload();
            mUploadCallback.onUploadCanceled();
            mUploadCallback = null;
        }
        mUploadState = STATE_NONE;
    }

I can create a PR if you'd like.
Thanks

[bgiori]

Hi Josef,

We could definitely have a conversation about the best order which the upload callbacks get called. I'll give you a quick rundown about the reasoning behind this behavior, and If you have some ideas about callback ordering or APIs in general, let me know.

Since cancellation could happen at any time while the upload is in progress, the cancelUpload callback will only set the state flag to NONE rather than resetting the upload and calling the cancel callback. The state then gets checked in the upload response callback which calls the progress callback first, then handles the cancellation:

private final McuMgrCallback<McuMgrImageUploadResponse> mUploadCallbackImpl =
            new McuMgrCallback<McuMgrImageUploadResponse>() {
                @Override
                public void onResponse(@NotNull McuMgrImageUploadResponse response) {

                    // ...

                    // Call the progress callback.
                    mUploadCallback.onProgressChanged(mUploadOffset, mImageData.length,
                            System.currentTimeMillis());

                    if (mUploadState == STATE_NONE) {
                        LOG.debug("Upload canceled!");
                        resetUpload();
                        mUploadCallback.onUploadCanceled();
                        mUploadCallback = null;
                        return;
                    }

                    // ...

                }

The progress callback is called before the cancellation callback because the upload has progressed to that point. I believe we felt that we would be losing some information if upload cancelled immediately while a command was in transit, hence the reason why the cancelUpload() method does not immediately "cancel" the upload when the upload is the UPLOADING state. However, I am not certain that this is the correct callback paradigm, as cancellation implies that the upload will not be continued, and therefore the user/UI probably doesn't care about one more packet of progress.

All that aside, simply making that change in your original post may have unintended consequences (e.g. null pointer exceptions). But if you feel strongly about immediate cancellation, I am happy to review a PR.

Proposed Solution (#32)

I currently have a PR open for refactoring and abstracting all transfers (uploads and downloads) outside of the individual managers (#32). This would replace the current upload/download implementations in ImageManager and FsManager and hopefully make fixing these type of issues easier in the future. The transfer abstraction PR is currently on hold but I should be able to get back to it sometime in the near future.

Brian

[joseffilzmaier]

Hi!

However, I am not certain that this is the correct callback paradigm, as cancellation implies that the upload will not be continued, and therefore the user/UI probably doesn't care about one more packet of progress.

Yes, for our application the next transferred frame does not matter at all once the decision to cancel the transfer is made. Therefore i would have guessed that cancelUpload() immediately does what it says. To achieve this i am currently using pauseUpload() followed by cancelUpload() (Working nicely so far)

All that aside, simply making that change in your original post may have unintended consequences (e.g. null pointer exceptions). But if you feel strongly about immediate cancellation, I am happy to review a PR.

I am wrapping the ImageManager class upload callbacks into an RxJava observable and once the observable is disposed i am not allowed to receive any more callbacks as otherwise i get UndeliverableExceptions. Since there is also no way to clear the callback i either have the choice to cancel the upload (expecting no more callbacks except the onUploadCancelled() one) or to create some weird conditions to not use the results of the callback once the observable is disposed.

I currently have a PR open for refactoring and abstracting all transfers...

Sounds like a good idea, however i have glanced through your PR and i think that your PR is unrelated to the issue i raise here.

So, design wise i would still vote for an implementation that immediately cancels transfers when using the cancelUpload() method.

Thank you for your time!

Cheers Josef