Repository navigation
Consider API responses to api/ping with text/html header as failures - #2396
Conversation
| final response = await http.get(uri).timeout(const Duration(seconds: 1)); | ||
| if (response.statusCode != 200) { | ||
| if (response.statusCode != 200 || | ||
| // When running locally with `flutter run`, missing request return a 200 |
There was a problem hiding this comment.
Is this a bug (not returning a 404) in the flutter tools' server?
There was a problem hiding this comment.
I would have thought so (though since it's a dev server I'm not sure how correct it's expected to be). The issue linked in the comment is one I've raised in flutter/flutter assuming it's a bug. If it is (and it's fixed or we can fix it), we can revert this (it doesn't impact end users that would don't run DevTools using flutter run).
I think in the meantime we should prevent the error though (it's been quite confusing as I've been trying to track down another error, that this one occurs some small delay after loading the page!)
There was a problem hiding this comment.
I'll add a TODO about reverting once fixed (and if it's decided not to, I think we could inspect the response of ping to ensure it looks like the server instead).
There was a problem hiding this comment.
Turns out this was working as intended. It's done to support PathUrlStrategy in Flutter (hash-less URLs) - so if you press Refresh on any route, it'll still serve up your app.
I think that's fair, so if we need a fix long-term, I think it would be better to just the status code is 200 and the content body is a well-known response for ping. Sound reasonable?
There was a problem hiding this comment.
They're returning http 200 for missing resources? That doesn't sound great wrt good http patterns.
Perhaps work around that - assume we'll get an missing resource code from most servers, and do an explicit sniff test for the flutter_tools response (200 and some known content from flutter_tools' response)?
Note that they could still serve up html content in their 404 response if they wanted to.
There was a problem hiding this comment.
They're returning http 200 for missing resources? That doesn't sound great wrt good http patterns.
There's a note in flutter/flutter#67053 (comment) about it, but it's to support hash-less URLs on the dev server. Without this, if you navigate to /foo in your Flutter web app and then press Refresh, you'd get a 404 because there isn't really a /foo locally on disk (it had previously been handled by the client app).
I can understand this (I've often set up local servers with a "catchall" to handle refreshing when there are custom routes - though if the production server isn't set up the same, it can bite you) though it does introduce issues like this (I'm not sure which is the greater issue).
do an explicit sniff test for the flutter_tools response (200 and some known content from flutter_tools' response)?
Yep, that's what I was thinking - I think we return 'OK' or something. Checking for that seems like it woul dbe more reliable (we know for sure we're seeing our server).
Note that they could still serve up html content in their 404 response if they wanted to.
Yeah, that could work. I don't know if there would be any other consequences of that - I'll mention it in the issue and see what they think.
When running locally in Chrome without the server, I see errors trying to connect to the SSE backend. This seems to be because of flutter/flutter#67053 - the call to
api/pingreturns a 200 status and that's considered successful.I added a check for text/html (which is the type it returns). The real API responses are
text/plain(I'm not sure if that's correct, but it's at least not - and should likely never be - HTML).