racket / web-server

Other
90 stars 47 forks source link

Avoid adding `content-length` if it already exists #132

Closed elibarzilay closed 8 months ago

elibarzilay commented 9 months ago

This is going to be a long story. It leads to a bad bug, which took me probably more than 5 years to debug since I had to do that indirectly through enthusiastic students that are hard to find.

The end result of the below is that download urls from the handin server which go through Apache which serves HTTP2 just don't work at all in Safari, which will retry the download many times and eventually give up. The (easy + verified) fix is as the title says, to avoid duplicating the header if it's already included in the user call. Pointers below.


More details:

The thing that happens --- as much as I can guess --- is that the handin server creates a download URL with a Content-Length header in this bit of the code, and the web server blindly adds a second Content-Length in this code through this function which doesn't check if it exists in headers before adding it (and therefore the fix would be in this place).

The nasty aspect of the resulting brokenness has several aspects that makes it insane to figure it out:

Since this is so obscure, and requires a macos to see the effect, I just told students to use chrome instead. (And given my vocal opinions about apple, I had to explain that this is not intentional --- I'm not going to touch it, but I didn't intend to stand in-between some students' pistols and their respective feet.) Every couple of semesters or so, some student would try to debug it, fail to see the problem, get on a zoom with me so I can have the pleasure of looking at HTTP headers while scratching my own head. That went on for a number of years.

Finally, I had a student that happily jumped into a wireshark experiment, and finally caught the problem.

I verified that removing the header creation in the handin code works -- at least as much as using curl shows (no error when it doesn't create the header).

CC @mflatt -- removing that bit from web-status-server.rkt will resolve it now, but fixing the web server would be nice too.