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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Total Lines of Code gives HTTP 408 periodically #7889

Closed
ascopes opened this issue Apr 29, 2022 · 14 comments
Closed

Total Lines of Code gives HTTP 408 periodically #7889

ascopes opened this issue Apr 29, 2022 · 14 comments
Labels
question Support questions, usage questions, unconfirmed bugs, discussions, ideas

Comments

@ascopes
Copy link

ascopes commented Apr 29, 2022

Are you experiencing an issue with...

shields.io

馃悶 Description

The lines of code badge has been giving a 408 intermittently for the past few days.

When this occurs, the badge does not render and shows a "missing image" icon from the browser, rather than providing an error badge.

Seems to be intermittent though. Chrome reported this was a 408 response. None of the other badges are doing this.

馃敆 Link to the badge

https://img.shields.io/tokei/lines/github/ascopes/java-compiler-testing

馃挕 Possible Solution

No response

@ascopes ascopes added the question Support questions, usage questions, unconfirmed bugs, discussions, ideas label Apr 29, 2022
@calebcartwright
Copy link
Member

The Shields.io service provides badges which are based on data we fetch from some external source (we don't provide CI, package registry, code counting, etc. capabilities ourselves). As such we're directly impacted by upstream issues and latency in those external sources and there's not really anything we can do about it.

There's been recurrent issues in the Tokei service for the last several months, and I don't think there's really anything new to discuss nor investigate here. Refs: #7569 XAMPPRocky/tokei#881

@ascopes
Copy link
Author

ascopes commented Apr 29, 2022

Should this not propagate as a 500 or 503 response in this case, since it is occuring outside the control of the client? Since a 408 usually suggests the server wants to kill the connection because the client is not making use of it, when in reality the client is awaiting a response from the server (at least, according to Mozilla). 4xx suggests the user has made a mistake, not the backend.

@calebcartwright
Copy link
Member

No, I do not think we should categorically issue a 503 nor 500 response status code to our users when we're stuck waiting on the tokei service to respond to us so that we can then actually build and return the badge to our client.

@ascopes
Copy link
Author

ascopes commented Apr 29, 2022

So are we saying this error is occurring on the client then (e.g. my browser), since 4xx implies the client is the cause of an issue, rather than 5xx which would indicate it is outside the user's control?

Going by the definition in RFC-7231, this response code is saying I took too long to send a payload to shields.io, right? That doesn't appear to be the case here going by what you have said, unless I am missing something? Trying to work out whether this is an issue my side or not (since I don't understand the architecture behind shields.io)

@calebcartwright
Copy link
Member

I'm not sure what your objective is here anymore, so let's take a step back.

Was your goal to try to get an understanding of the issues with the Tokei badge, or do you want to have a more holistic discussion about specific response codes orthogonal to/independent of the tokei badge essentially being broken due upstream issues?

In general we're receptive to the latter types of discussions, even if they trend towards pedantry and away from pragmatism. I'm open to considering alternative behavior, but it's also not clear to me what the practical benefits would be; these badges aren't going to render.

I'll also note it would be particularly helpful if you could capture the behavior you're describing at the point you're seeing it, as it's not reproducible currently. Additionally, worth keeping in mind that with the 408 code in particular there's plenty of literature that notes it can be sent by the server in cases beyond a client/request specific issue, despite that being one of the typical differentiators between 4xx and 5xx codes.

@ascopes
Copy link
Author

ascopes commented Apr 29, 2022

Right, I follow now. I was getting confused as to whether this was something being triggered by the request I was sending that I needed to change (going by that status code), or whether it was outside my control and just a general issue.

I don't have any concrete way of reproducing this as it seems to occur randomly. It also seems to be reporting 0 lines of code in other scenarios randomly too, which is the case currently:

IMG_20220429_170524

What would you suggest would be a good course of action? Is it best to stop using that badge if this is an ongoing issue as you mentioned earlier, or is there an alternative?

@calebcartwright
Copy link
Member

What would you suggest would be a good course of action? Is it best to stop using that badge if this is an ongoing issue as you mentioned earlier, or is there an alternative?

Unfortunately I don't think there are any good courses available. There's some great devs that worked on Tokei but it's been persistently problematic that at some point we may have to consider dropping the badge entirely. We're also not aware of any alternatives either; code counting was a long-requested badge, with Tokei at the time seeming like the first viable candidate to deliver those capabilities.

Personally, I'd probably stop using it for now and just subscribe to the upstream issues in the hopes that they get things sorted. Alternatively, if you ever do come across a good service/platform that can count code please do let us know as we'd love to be able to provide such a working badge 馃榿

Thanks for reaching out, but going to close as I think we've reached a logical conclusion and have no concrete action items for us, though certainly feel free to add any additional comments or questions!

ascopes added a commit to ascopes/java-compiler-testing that referenced this issue Apr 29, 2022
Remove LOC badge (ref: badges/shields#7889)
@paulmelnikow
Copy link
Member

From an API and usability perspective, we could consider issuing a 5xx when upstream services time out. With only a few exceptions it seems like it is not something the client can fix (even if, in most cases, it's not something Shields can fix either).

@calebcartwright
Copy link
Member

From an API and usability perspective, we could consider issuing a 5xx when upstream services time out. With only a few exceptions it seems like it is not something the client can fix (even if, in most cases, it's not something Shields can fix either).

Does issuing a 500 response really help the user that much though? From a badge perspective I think the most helpful thing would be a standard 200 response with a badge message indicating something to the effect of

@ascopes
Copy link
Author

ascopes commented Apr 29, 2022

if a 5xx is passed with a badge in the response, do browsers still render that?

@calebcartwright
Copy link
Member

calebcartwright commented Apr 29, 2022

if a 5xx is passed with a badge in the response, do browsers still render that?

I'd have some philosophical objections to returning a 5xx style response with a body that's a "successful" badge, but i think a bigger consideration for us would be Camo's behavior given that's what's handling things within the GitHub markdown views. I could be mistaken, but I don't remember it attempting to render content from failed requests (as I would presume it would anticipate the body of a failed request containing error-related content)

@paulmelnikow
Copy link
Member

Right, probably the more useful thing for the user would be a badge with an error, rather than an empty response.

The other thing to consider about camo (and browsers in general) is what caching behavior we want. How long do we want these timeout errors to be cached? It's not clear the status code is the best way to control that; probably adjusting the cache header would be better.

@chris48s
Copy link
Member

chris48s commented May 1, 2022

Its not merged yet but #7877 will move us to just dropping the connection in the case of a timeout instead of returning a 408 response. As I understand it so far, this is basically a non-configurable behaviour of express so this may end up moot fairly shortly.

@calebcartwright
Copy link
Member

As I understand it so far, this is basically a non-configurable behaviour of express so this may end up moot fairly shortly.

Can't speak to configurability/controllability of Express on that front, though something like what I suggested in #7889 (comment) would still be possible no? I.e. incorporate new features into how the badge server issues requests to bail out early (earlier than whatever max timeout Express is enforcing) and give the user back some kind of error badge?

To be clear I'm not really pushing for that as it seems to be more of an edge case anyway, but I do think it would still be on the table regardless of what web/server framework we use

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Support questions, usage questions, unconfirmed bugs, discussions, ideas
Projects
None yet
Development

No branches or pull requests

4 participants