Page MenuHomePhabricator

Add error detection to HTTP fetch in foreign resources checker before gzipping
Closed, ResolvedPublic

Event Timeline

I'd prefer to decline this as it is working fine. I assume this task originated from my suspicion that the Phar logic was causing issues with the Moment.js tar unpacking in February - where we saw a consistent "Segmentation fault" error.

I suspected at the time it may've been due to some logic there being intended for Phar-specific logic and not regular tarballs.

However, I was wrong about that.

  1. The problem was that we were feeding it an HTML body from a HTTP 302 response from GitHub, not an actual tarball.
  2. It turns out the tarball/phar unpacking logic is pretty well separated with parameters to trigger one or the other.

I don't mind making the code more elaborate later on, but in its currently form I'd rather not fix something we haven't found to have any kind of issue :)

Maybe detach it as a blocker and reduce to Lowest? Proper error state management is valid, just not a high need.

Legoktm changed the task status from Open to Stalled.Jun 21 2019, 6:27 AM
Legoktm added a subscriber: Legoktm.

Unclear whether this needs fixing at all...

Krinkle claimed this task.

Proper error state management is valid, just not a high need.

Agreed. This was done meanwhile (two days after the filing of this task) at https://gerrit.wikimedia.org/r/492215 / ed5f7a6d2719c857.

The checker now validates the HTTP status code before trying to feed response body to the decompressor, thus avoid the weird corruption we saw, and offering useful error messages that would have prevented the problem that led to this report at c96f90cb2b44.

Krinkle renamed this task from Change the foreign resources checker to use a real gzip library, not the PHP-internal Phar stuff to Add error detection to HTTP fetch in foreign resources checker before gzipping.Jun 21 2019, 10:14 PM