1371489 - taskgraph uses new connections to index.taskcluster.net after a 404

Status: RESOLVED FIXED

(Taskcluster :: General, enhancement)

Description

[Mike Hommey [:glandium]]

cf. the logs from a decision task on try:

task 2017-06-07T22:51:20.495815Z] "GET /index/v1/task/gecko.cache.level-3.toolchains.v1.linux64-clang.d616ce8d88e325714d2d490ed75333bf6772e6cc0b510769dd9226c717f940e2 HTTP/1.1" 404 449
[task 2017-06-07T22:51:20.497298Z] Starting new HTTP connection (2): taskcluster
[task 2017-06-07T22:51:20.696601Z] "GET /index/v1/task/gecko.cache.level-2.toolchains.v1.linux64-clang.d616ce8d88e325714d2d490ed75333bf6772e6cc0b510769dd9226c717f940e2 HTTP/1.1" 404 449
[task 2017-06-07T22:51:20.698184Z] Starting new HTTP connection (3): taskcluster
[task 2017-06-07T22:51:20.915775Z] "GET /index/v1/task/gecko.cache.level-1.toolchains.v1.linux64-clang.d616ce8d88e325714d2d490ed75333bf6772e6cc0b510769dd9226c717f940e2 HTTP/1.1" 200 239
[task 2017-06-07T22:51:20.916358Z] optimizing `toolchain-linux64-clang`, replacing with task `SHSZL0OYRdmlV-elrk5kxQ`
[task 2017-06-07T22:51:21.151152Z] "GET /index/v1/task/gecko.cache.level-3.toolchains.v1.linux64-clang-tidy.a64debfdbd29c9b355044a284de9486aa0ced259442db8b5febba277a674f247 HTTP/1.1" 404 454
[task 2017-06-07T22:51:21.152537Z] Starting new HTTP connection (4): taskcluster
[task 2017-06-07T22:51:21.396985Z] "GET /index/v1/task/gecko.cache.level-2.toolchains.v1.linux64-clang-tidy.a64debfdbd29c9b355044a284de9486aa0ced259442db8b5febba277a674f247 HTTP/1.1" 404 454
[task 2017-06-07T22:51:21.398548Z] Starting new HTTP connection (5): taskcluster
(...)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1371489 - Reuse HTTP connections after HTTP errors.

When a response has unconsumed content, requests closes the connection
instead of releasing it to the connection pool, such that a new
connection has to be created when another request to the same host is
done within the same Session.

When calling response.raise_for_status, the response content is left
unconsumed, meaning that without manually consuming the content first,
every time raise_for_status raises, a connection is closed.

Review commit: https://reviewboard.mozilla.org/r/147354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147354/

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8875947 [details]
Bug 1371489 - Reuse HTTP connections after HTTP errors.

https://reviewboard.mozilla.org/r/147354/#review152356

::: taskcluster/taskgraph/util/taskcluster.py:52
(Diff revision 1)
>      For other types of content, a file-like object is returned.
>      """
>      response = _do_request(get_artifact_url(task_id, path, use_proxy))
> +    # Consume content before raise_for_status, so that the connection can be
> +    # reused.
> +    response.content

This function supports returning raw files, which won't work if we've already read the content. I don't think that functionality is used (at least, I don't see it used in https://hg.mozilla.org/mozilla-central/rev/c9dabe672fd8), so it could probably be removed.

In which case, why not just move `response.content` to `_do_request`?

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8875947 [details]
Bug 1371489 - Reuse HTTP connections after HTTP errors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147354/diff/1-2/

Comment 4

[Mike Hommey [:glandium]]

(In reply to Dustin J. Mitchell [:dustin] from comment #2)
> This function supports returning raw files, which won't work if we've
> already read the content. I don't think that functionality is used (at
> least, I don't see it used in
> https://hg.mozilla.org/mozilla-central/rev/c9dabe672fd8), so it could
> probably be removed.
> 
> In which case, why not just move `response.content` to `_do_request`?

In the end, I didn't want to decide whether to remove the raw file thing from get_artifact (possibly returning response.content, but then that'd be memory consuming and footgun'y), so I went with just doing it in case of errors, and moving it to `_do_request`.

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8875947 [details]
Bug 1371489 - Reuse HTTP connections after HTTP errors.

https://reviewboard.mozilla.org/r/147354/#review155848

thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9933a2e58516
Reuse HTTP connections after HTTP errors. r=dustin

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9933a2e58516