exec: fix file handle leak with container.exec_* APIs

[Lekensteyn]

Requests with stream=True MUST be closed or else the connection will
never be returned to the connection pool. Both ContainerApiMixin.attach
and ExecApiMixin.exec_start were leaking in the stream=False case.
exec_start was modified to follow attach for the stream=True case as
that allows the caller to close the stream when done (untested).

Tested with:

# Test exec_run (stream=False) - observe one less leak
make integration-test-py3 file=models_containers_test.py' -k test_exec_run_success -vs -W error::ResourceWarning'
# Test exec_start (stream=True, fully reads from CancellableStream)
make integration-test-py3 file=api_exec_test.py' -k test_execute_command -vs -W error::ResourceWarning'

After this change, one resource leak is removed, the remaining resource
leaks occur because none of the tests call client.close().

Fixes #1293
(Regression from #1130)

Signed-off-by: Peter Wu pwu@cloudflare.com

---

Relevant documentation:
https://2.python-requests.org//en/master/user/advanced/#body-content-workflow

Note: it is not easy to write a test to catch this issue. For one, lot of other tests also suffer from the resource leak issue (client.close() not being called). The other issue is that this error is only written to stderr.

While pytest supports the capsys fixture to capture stderr, this is not easily possible due to the use of unittest which does not allow fixtures as function parameters: https://docs.pytest.org/en/latest/unittest.html
(Potential workarounds include using request.getfixturevalue, pytest-dev/pytest#4143 or manually changing sys.stderr.)

[jgfoster]

This fixes the problem I observed and I hope it gets incorporated soon!

[inkychris]

Are there any technical reasons why this hasn't been merged, or has it just slipped under the radar? Tentatively tagging @shin- in case they may be able to help!

[Nitrooo]

@milas can you please review and possibly merge this fix? Thank you.

[milas]

Thanks for the PR & fix!