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

Add timeout for network requests #18

Open
felipenmoura opened this issue Aug 4, 2016 · 14 comments
Open

Add timeout for network requests #18

felipenmoura opened this issue Aug 4, 2016 · 14 comments

Comments

@felipenmoura
Copy link
Member

We should definitely have a way to define for how long the user will stay waiting for a request before we kill it and deliver something else (like a custom page or default value).

@felipenmoura
Copy link
Member Author

This issue needs more discussion!
What should happen when a request reaches its timeout? Should it respond with its cached version? Should it fail as a network failure? Should it have a fallback applied(like a redirect, maybe)?

@jaydson
Copy link
Member

jaydson commented Sep 13, 2016

That's a must have feature, indeed.
IMHO, I think we need to provide a way for developers to decide what should happen.
Perhaps the default behaviour should be a HTTP 408 response, and like you suggested, a fallback can be applied.

Any other thoughts?
//cc @UltCombo

@UltCombo
Copy link
Contributor

IMHO, I think we need to provide a way for developers to decide what should happen.

👍
There are several use cases and no "one size fits all" solution, as far as I can see. I can imagine use cases were responding with cached resources, redirecting and failing would make sense, so perhaps having options for all these use cases would make sense. Also, I believe one may want to combine these behaviors, so, for instance, you may want to respond with a cached version for unimportant things like images, but fail or redirect for critical files like JS/CSS. Therefore, allowing the user to choose specific behaviors for specific MIME types/file extensions/URLs/URL patterns would make sense, I believe.

Also, responding with a cached version seems very appealing, but it also has a huge problem with _predictability_. Let's examine a use case. Let's say the timeout is defined as 8s and everything is set to respond with a cached version if the network times out. It may just happen that an updated CSS file is downloaded at 7.8s but a JS file times out and the user gets an outdated version of it. The application will be in an inconsistent state and likely broken.

For an application-wide "show cached version if connection is slow" use case, I guess the ideal solution would be to apply the timeout only to the first request in the page (e.g. the HTML file), if it is too slow then flip the switch and serve all resources from cache. This would ensure the application is always in a consistent state, the user gets either the latest version or the old version—not a bit of each—thus ensuring predictable behavior.


I'm just throwing some ideas out here, hopefully it's some food for thought.

I've just spent a few minutes thinking about this issue and came up with so many options, that's one of the reasons I usually dislike declarative abstractions—they are never flexible enough, and when they try to be they end up becoming so goddamn bloated with feature creep that it is often easier to write the imperative code for it than trying to figure out all of the declarative abstraction's options and values. So, please, give some thought about what is really worth implementing before trying to cover every single use case. Balance is very important here. 😄

@felipenmoura
Copy link
Member Author

Nice.
Indeed, the balance is very important. Declarative structures some times may become monsters and that's definitely not the goal here.

I actually believe we have an easy way to allow developers to decide what is the best solution as well as keeping the configuration object quite simple.

Only two actions required (and one already exists):

  1. We can actually use, as mentioned by @jaydson, a 408 header.
    If there is any rule defined for 408 headers, they shall be applied (this already works).
"timed-out-images": {
    "match": {
        "extension": ["png", "jpg", "jpeg", "gif"], // could be any match
        "status": 408 // that has a 408 status
    },
    "apply": {
        "fetch": "another-source.jpg" // to something else
    }
},

But as @UltCombo mentioned, there are cases in witch we may prefer to have it from cache, so:

  1. We create a timeout configuration:
"images": {
    "match": {
        "extension": ["png", "jpg", "jpeg", "gif"] // again, any match
    },
    // it only makes sense if it is online-first or if it is the first time with "fastest" strategy
    "strategy": "online-first",
    // the new feature
    "timeLimit": {
        timeout: 2000,
        // default is true, if false, it becomes a 408 (which may fall into
        //  another rule) like the previous one
        useCache: false
    },
    "apply": {
        // cache could still be used in case of 404
        "cache": {
            "name": "cached-images",
            "version": "1"
        }
    }
}

In case timeout is defined, useCache is false and there is cache in apply (the example above):

  • When timing out: will become 408, and if there is any rule for 408, it should be applied.
  • When not found, it becomes 404 and goes for the cache(once it is online-first), if not in cache, rules for 404 will be applied.

In case timeout is defined, useCache is true but there is no cache in apply:

  • When timing out, it becomes 408 and rules for it may be applied
  • when not found, it becomes 404 and rules for it may be applied

If timeout is defined and there are no rules to treat 408 requests, it ends up like a failed 408 request, indeed.

I believe it covers all possible situations, by adding one simple configuration object to it.

By the way, DSW already works with a --format js flag...just not stable enough for shipping. With this, instead of having the dswfile.json you would have dswfile.js, which will be an IIFE that would return a valid object.

@UltCombo
Copy link
Contributor

Nice! Very throughout explanation. 😄

I'm just not sure what's the use case for responding with a cached version when the resource is not found (404). If the request reaches the server and the server responds with 404, the resource likely doesn't exist anymore—for what use cases would it make sense to use a cached version of something that no longer exists?

@felipenmoura
Copy link
Member Author

Well, it may still exists but your device may have lost its connection.
Delivering the cached version will allow your page to work offline.

@UltCombo
Copy link
Contributor

Well, I thought you were talking about a 404 response—if you get a 404 back then it means the connection to the server is working and the resource doesn't exist anymore. I believe connection errors would fail the fetch request instead of returning a 404 response.

@felipenmoura
Copy link
Member Author

Nope...fetch will not fail, instead, it will resolve with the Response object with status 404.

fetch('/non-existing-page').then(function(response){
    console.log(response); // Response { status: 404, statusText: "Not Found" }
});

@UltCombo
Copy link
Contributor

I guess we may be having some communication problem. Let's try again.

Well, it may still exists but your device may have lost its connection.

Here you are talking about connection problems (e.g. device is offline), right? That's what I'm trying to address. However, it seems you are mixing it up with 404, which is just a server response—that means fetch request reached the server successfully and the server responded with a "not found" status.

@UltCombo
Copy link
Contributor

To be more clear, your comment above mentions handling 404s with cached responses, but then you said the cache would be used to allow the page to work offline. This doesn't seem to add up for me as those are two very different cases as far as I can see.

@felipenmoura
Copy link
Member Author

hm, I understand.
The two situations you mentioned would be for failed requests when offline, and failed requests for non-existing pages.

But I see only two possible outputs. The cashed version, or the failure itself.
If the requests fails with a 404 (let's say, for a page or content that is missing now, for some reason) and the user has the data cached, why not to use it? I mean, the user did requested for that content, and the content IS stored in the user's device...should we simply deliver a 404 failure, then?! Also, 404 requests could be treated in other rules and, for things that are "live" and should not be cached, the cache: false rule should be used.

@UltCombo
Copy link
Contributor

The two situations you mentioned would be for failed requests when offline, and failed requests for non-existing pages.

Yes, exactly.

If the requests fails with a 404 (let's say, for a page or content that is missing now, for some reason) and the user has the data cached, why not to use it?

I believe this may happen under a few circumstances:

  • Developer error. The application was updated, but it still links to resources that no longer exist. The service worker has no right or wrong behavior here; it is a developer error. Perhaps failing the request with 404 would help the developer to spot this problem more easily without having to clear the cache or reinstall the service worker.
  • Inconsistent application state. Like I've mentioned earlier, this can happen if the application fetches some updated resources from the network and some outdated resources from the cache. The application will probably be broken independent of the service worker behavior in this case. I've suggested an alternative approach to avoid this issue earlier in this thread.
  • Intentionally removed content. Several applications such as forums and boards may remove illegal or otherwise inappropriate content such as images and pages, intentionally making them unavailable.

So yeah, I believe there are cases when the user may want to handle 404s in different ways, but we may be starting to get a bit off-topic. It seems DSW already has a way to set the handling for specific HTTP status codes, right? I believe that can be used to determine how 404s will be handled. I'm just not sure how the timeLimit's useCache option is supposed to work with all of this.

@felipenmoura
Copy link
Member Author

Good points, @UltCombo , indeed.

Yes, it's possible to define different rules for different 404 cases and it is, a little bit off topic to timeLimit. Even though, it's interesting to talk about it and discuss the possibilities (this way, we can be sure we are covering the most alternatives as possible). In this case, I think the discussion is more about the default 404 behaviour.

Anyway, are we in the same page about the timeLimit, timeout and useCache I proposed?

@UltCombo
Copy link
Contributor

I think the discussion is more about the default 404 behaviour.

I guess you mean "default fail behavior", right? I'm not sure if 404 qualifies as a failed request, but as long as the user can customize the handling for such cases it doesn't really matter.

Anyway, are we in the same page about the timeLimit, timeout and useCache I proposed?

I'm just not sure how useCache is supposed to work. Wouldn't it be easier to have an apply property in the timeLimit object to allow customizing the handling for timed out requests? This way, it can follow the same standard as the other error conditions.

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

No branches or pull requests

3 participants