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

Rx and how to know if Request has completed #247

Open
giniedp opened this issue Dec 12, 2018 · 5 comments
Open

Rx and how to know if Request has completed #247

giniedp opened this issue Dec 12, 2018 · 5 comments
Milestone

Comments

@giniedp
Copy link

giniedp commented Dec 12, 2018

When overriding a Request we have the power to override the following methods

  • deliverError
  • deliverResponse

I want to write a custom request that enables reactive programming. I want my request to emit values onNext, emit errors onError and emit completenes onComplete. My first attempt is the following code.

    @Override
    public void deliverError(VolleyError error) {
        super.deliverError(error);
        this.emitter.onError(error);
        this.emitter.onComplete();
    }

    @Override
    protected void deliverResponse(R response) {
        super.deliverResponse(response);
        this.emitter.onNext(response);
        // is this a cached response and there will be another network response
        // or is it a network response and i can safely call onComplete()
        this.onComplete();
    }

Since deliverResponse is called for both cached and network responses and there is no way to distinguish between them (or is there?) we can not reliably call onComplete after delivery.

Maybe i am missing something.

@giniedp
Copy link
Author

giniedp commented Dec 12, 2018

so i now tried to use a custom implementation of ExecutorDelivery as suggested in #113

Looking at the implementation of ResponseDeliveryRunnable it looked promising to actually have a custom implementation of ResponseDeliveryRunnable. However, then we'd need access to package private methods like finish and deliverResponse.

As suggested here #70 next attempt is to extend ExecutorDelivery and enrich it with a custom runnable that would run here. This would work for postResponse but not for postError since there is no way to add a custom runnable in that case.

so i change my custom request implementation to

    @Override
    public void cancel() {
        super.cancel(error);
        this.onComplete();
    }

    @Override
    public void deliverError(VolleyError error) {
        super.deliverError(error);
        this.onError(error);
        this.onComplete();
    }

    @Override
    protected void deliverResponse(R response) {
        super.deliverResponse(response);
        this.onNext(response);
    }

and then implement the custom ResponseDelivery like this

public class CustomResponseDelivery extends ExecutorDelivery {

    public CustomResponseDelivery(Handler handler) {
        super(handler);
    }

    public CustomResponseDelivery(Executor executor) {
        super(executor);
    }

    @Override
    public void postResponse(com.android.volley.Request<?> request, Response<?> response, Runnable runnable) {
        super.postResponse(request, response, new PostRunnable(runnable, request, response));
    }

    private static class PostRunnable implements Runnable {

        private final Runnable otherRunnable;
        private final Request<?> request;
        private final Response<?> response;

        PostRunnable(Runnable otherRunnable, Request<?> request, Response<?> response) {
            this.otherRunnable = otherRunnable;
            this.request = request;
            this.response = response;
        }

        @Override
        public void run() {

            if (request instanceof MyRequest && (request.isCanceled() || !response.intermediate)) {
                ((MyRequest) request).onComplete();
            }

            if (otherRunnable != null) {
                otherRunnable.run();
            }
        }
    }
}

This is all pseudocode so i still have to fine tune the idea and test it. It would be very nice if we could have control over methods like request.finish() or similar.

@jpd236
Copy link
Collaborator

jpd236 commented Dec 12, 2018

Yeah, I think this is a bit tricky with current APIs, since you want to see Response#intermediate, but this detail is abstracted away unless you are implementing a custom ResponseDelivery as you've done.

Another approach you might explore would be to look at Request#getCacheEntry. When an intermediate response is delivered from the cache, getCacheEntry will return the CacheEntry. For final responses (either from the cache, or the network), it'll return null. So, when the first response comes in, either getCacheEntry is null, in which case you should get no further callbacks, or it is not, in which case you set a flag to expect one more callback. This is a bit hacky, though, and I'd hesitate to rely on it, even if it is simpler.

Leaving this open as a longer-term feature request to more easily know whether a response is intermediate or not. Unfortunately I'm not sure if there will be an easy way to do this without breaking existing APIs, since there's currently no object with which we can associate the information that is passed where needed.

@jpd236 jpd236 added this to the 2.0.0 milestone Dec 12, 2018
@uhager
Copy link
Contributor

uhager commented Dec 13, 2018

When an intermediate response is delivered from the cache, getCacheEntry will return the CacheEntry. For final responses (either from the cache, or the network), it'll return null. So, when the first response comes in, either getCacheEntry is null, in which case you should get no further callbacks, or it is not, in which case you set a flag to expect one more callback.

It could also be an expired cache entry that gets added to the request (for not-modified responses). In that case, the response would be the final, network response but the request still has the entry.
However, in Request#parseNetworkResponse, you can check the NetworkResponse's networkTimeMs. For a cache hit, it's 0.
So:
0 network time + no cache entry: fresh cache hit, only one response
0 network time + cache entry: soft TTL refresh, intermediate response from cache
non-0 network time: this is always the last response, could be a soft TTL refresh, expired entry, or no entry in cache.

But if you just want to do something when the request is finished, I think there is an easier way, I haven't used it so I'm not quite sure: If you build Volley from the current version, you can add a RequestEventListener and listen for RequestEvent.REQUEST_FINISHED which the RequestQueue sends in finish.

@giniedp
Copy link
Author

giniedp commented Dec 13, 2018

an ability to set acompleted callback on the request would be enough and would not introduce any breaking changes. For this use case i dont need to know whether it is an intermediate response or not (although this is a nice to have) if i can be notified when the request is done.

RequestEvent.REQUEST_FINISHED sound promising. I'll try that

@giniedp
Copy link
Author

giniedp commented Dec 13, 2018

RequestEvent.REQUEST_FINISHED is exactly what i would need. However even with current version same is possible via queue.addRequestFinishedListener. Works perfect for what i need albeit not very elegant since onComplete is now implemented in a very distinct place than onNext and onError

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

No branches or pull requests

3 participants