Page MenuHomePhabricator

JJB qunit macro ignores curl error exit code
Closed, ResolvedPublic

Description

When investigating T99775 we found out a QUnit job to have an erroring curl command which does not terminate the build:

curl --include http://.../Special:BlankPage | head -n42
(23) Failed writing body

As pointed by @JanZerebecki:

Regarding the curl error: curl --fail only ommits any output in the HTTP status failure case. In sh false | true will be true. Bash has the option pipefail to change this to more expected behaviour. However in this case I think just omitting the tail is fine.
The curl error was also happening when all tests pass, see e.g.: https://integration.wikimedia.org/ci/job/mwext-Wikibase-qunit/9770/consoleFull

So we need set -o pipefail and die out with a nice message that points people to the error logs artifacts.

Details

Related Gerrit Patches:
integration/config : masterqunit: Remove obsolete 'tac|tac' hack

Event Timeline

hashar raised the priority of this task from to Needs Triage.
hashar updated the task description. (Show Details)
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 20 2015, 11:22 PM
hashar renamed this task from JJB qunit macro ignore curl error exit code to JJB qunit macro ignores curl error exit code.May 20 2015, 11:22 PM
hashar set Security to None.

Note: qunit jobs are actually passing despite the error so failing the curl command would potentially highlights some different issue and break all qunit jobs.

hashar triaged this task as Low priority.Jun 5 2015, 12:23 PM

The curl statement are merely there to let us manually check the site is properly configured. curl error 23 is

Write error. Curl couldn't write data to a local filesystem or similar.

So stdout would not be writable, maybe because head would close the pipe after 42 lines are read. Maybe we can insert cat in between.

with curl|tac|tac I am not sure whether it will still exit non 0. I guess it is now able to write properly to the pipe and exit once done. So potentially we can add set -o pipefail to abort when there are other errors.

Krinkle closed this task as Resolved.EditedSep 2 2015, 7:41 PM
Krinkle claimed this task.
Krinkle added a subscriber: Krinkle.

I think you misunderstood the original problem. There is no error in the cURL request, nor is there a problem in MediaWiki or QUnit. It's working fine.

The only problem was that the head command closes the read stream as soon as it has enough content (42 lines; as expected). cURL on the other hand doesn't anticipate closing of the read stream and tries to write the remaining html buffer to the stream as well (beyond line 42). The pipe rejects that and cURL subsequently reports "Failed writing body".

There was never a non-zero exit code exposed to begin with since head ignores that and returns 0 (just like echo). If the non-zero exit code were previously, then the job would already be aborted since we run with bash -e by default.

https://gerrit.wikimedia.org/r/235013 resolves this issue by exhausting curls output buffer before trimming it.

JanZerebecki reopened this task as Open.Sep 2 2015, 8:02 PM

I think hashar is right: If the curl were to fail then the bash script will not exit as we are missing a set -o pipefail for that, set -e is not enough for that.
If I remember correct the bug was filed because the script did not exit when curl saw a HTTP error even though curl then returns a non-zero exit code.

I think you misunderstood the original problem. There is no error in the cURL request, nor is there a problem in MediaWiki or QUnit. It's working fine.

The reason for this task is to have the job abort when curl exit code is non zero. The exit code is ignored because of the pipe (previously | head -n 42 , now |tac|tac.

To have bash fail inside the pipe, we need set -o pipefail. We could not enable it previously because of the Failed writing body error. Since it is now fixed, we can give a try at pipefail :-}

Can be reproduced with exit 1:

$ set +o pipefail; exit 1|cat; echo $?
0
$ set -o pipefail; exit 1|cat; echo $?
1
Krinkle added a comment.EditedSep 14 2015, 8:33 PM

This cURL request is not part of any test. I added it last year for debugging purposes. It's a build artefact essentially (similar to copying LocalSettings.php to the logs artefact directory).

If there is an error elsewhere, it will provide information. I can't imagine a reasonable scenario in which this cURL request fails but everything else passes.

Having said that, it's now RelEng's call.

Krinkle removed Krinkle as the assignee of this task.Sep 14 2015, 8:33 PM
Krinkle lowered the priority of this task from Low to Lowest.

We added |tac|tac| here to try to fix creating the curl error. Seems it doesn't work always: tac: write error. Seems we should just write it to disk, add it to the build artifacts and run head on the file.

Change 345392 had a related patch set uploaded (by Krinkle):
[integration/config@master] qunit: Remove obsolete 'tac|tac' hack

https://gerrit.wikimedia.org/r/345392

Change 345392 merged by jenkins-bot:
[integration/config@master] qunit: Remove obsolete 'tac|tac' hack

https://gerrit.wikimedia.org/r/345392

Krinkle closed this task as Resolved.Apr 4 2017, 10:31 PM
Krinkle claimed this task.