Calendar stops updating after FetchError, UnhandledPromiseRejectionWarning #237
Comments
|
That definitely is an error that should be fixed. I'll just need to acquire the skills to figure out how to handle this properly. So if anybody else knows how to fix this, I'm all ears. |
|
I dug in a bit more tonight. I found a way to reliably reproduce the problem and I came up with a fix: #238 The problem can be reliably reproduced on a Linux/OSX machine by adding a line like this to your /etc/hosts file. This makes all requests to fetch a calendar fail. The fix makes it so that the fetch will be attempted again after scanInterval, rather than failing permanently. |
|
Thanks! I guess with that PR we are closer to fixing it. The only gotcha is, that we need to differentiate between temporary and permanent errors. We cannot make continuous requests with a wrong password for example, since that could easily lock somebody's account. |
|
Thanks! I can imagine a few ways forward. Maybe the best would be to try to have the fetch call distinguish the type of HTTP error. You could reasonably fail permanently on any 4xx error (a client error), but continue with subsequent fetches on 5xx errors (a server error). What do you think? An easier alternative would be to try only once the first time you fetch a calendar, failing permanently if that doesn't work for any reason. There's your lockout mitigation. After that, you'd always do subsequent fetches, regardless of whether they're successful, presuming the errors are transient at that point. |
|
You're definitely onto something there. Generally differentiating on 4xx vs 5xx is a good first, but I'd say 401s should be handled specially always, since it's in each credential's nature to expire at some point (password changed, token expired etc). |
|
Oh yeah, true, I should've called out 401. I'm a total novice with Node and JavaScript generally, but if it's easy enough to get a status code from the failed call, what we're chatting about here sounds ideal. I fixed my PR, by the way. Apparently the linter didn't like it before, but I think it should be good now. |
|
@klaernie here's an enhanced version. Let me know what you think. I don't love how complicated the error checking becomes, but oh well :S. Also I'm not sure what would happen the user doesn't have the fetch module installed (I see you tried to make it an optional import). This code at least would need FetchError as an import from that module, though perhaps that could be worked around. const data = await fetch(url, opts).then(
// got a 3xx-5xx response code
(response) => {
if (response.ok) {
return response.text();
}
// This should actually be a much longer list, e.g. we'd definitely want 403 and especially 429.
// I'd really be tempted to just permanently fail on any 4xx error.
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses
if (response.status == 401 || response.status == 404) {
throw new Error(`[CALEXT2] calendar:${calendar.name} >> permanent fetch error ${response.status}. Will not try again.`);
}
Log.error(`[CALEXT2] calendar:${calendar.name} >> got retriable fetch error ${response.status}.`);
return null;
},
// got no response code, e.g. because we failed to connect to the server
(err) => {
if (err instanceof FetchError) {
Log.error(`[CALEXT2] calendar:${calendar.name} >> got retriable fetch error ${err}`);
return null;
}
// Fail permanently, since this was a very unexpected error.
throw err
}
); |
|
That looks really good to me. Throw that into a pull request, and given the tooling likes it I'll merge it. |
|
oh, and thank you very much for pouring time into this issue, @srabraham ! It is very much appreciated! |
|
Thanks! I've been using this module for a while now and I really like it, so I'm happy to help give back :D. I'll enhance that code snippet a bit in the next few days, then send a PR over to you. |
Hi there, thanks for your great work on this module!
I run a view that combines two Google calendars. Sometimes I'll see a transient error like the one below relating to one of the calendars, and that'll stop that calendar from getting updated entirely until MagicMirror is restarted. Notice how in the log, calendar:1 is not seen again after the error.
I can include my config too if you'd like. I make use of deduplicateEventsOn and a view transform function, but otherwise I think the config is fairly standard.
The text was updated successfully, but these errors were encountered: