Skip to content

Use one Session for the whole download#109

Merged
aclerc merged 1 commit into
v1from
v1-fix-session
Jun 25, 2026
Merged

Use one Session for the whole download#109
aclerc merged 1 commit into
v1from
v1-fix-session

Conversation

@aclerc

@aclerc aclerc commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Example failing tests: https://github.com/resgroup/wind-up/actions/runs/28163240915/job/83408733375?pr=100
One Session for the whole download so its connection pool (and every socket) is closed deterministically on exit.

@aclerc aclerc changed the title fix test flake Use one Session for the whole download Jun 25, 2026
@aclerc aclerc marked this pull request as ready for review June 25, 2026 12:13
@aclerc aclerc requested a review from Copilot June 25, 2026 12:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses leaked SSL sockets during Zenodo downloads by routing all HTTP requests through a single requests.Session that is closed deterministically at the end of the download.

Changes:

  • Wrap download_zenodo_data() in a single requests.Session() context and pass that session into _download_one_file().
  • Add an offline regression test that asserts all requests go through the shared session and that the session is closed exactly once.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
benchmarking/synthetic/sources/hill_of_towie.py Use one shared requests.Session for metadata + file downloads; _download_one_file now uses the provided session.
tests/benchmarking/synthetic/sources/test_hill_of_towie.py Adds a regression test ensuring all requests route via one Session and that it’s closed once.
Comments suppressed due to low confidence (1)

benchmarking/synthetic/sources/hill_of_towie.py:193

  • If result.raise_for_status() raises (e.g., 4xx/5xx), the streamed Response is never closed because the with (result, ...) block is not entered. That can still leak a socket/connection for error responses. Close the response before re-raising so the outer except requests.RequestException can handle it without leaving the connection open.
            result = session.get(
                _file_url,
                stream=True,
                timeout=(_CONNECT_TIMEOUT_S, _READ_TIMEOUT_S),
                headers=headers,
            )
            result.raise_for_status()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@aclerc aclerc merged commit b4b9416 into v1 Jun 25, 2026
1 check passed
@aclerc aclerc deleted the v1-fix-session branch June 25, 2026 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants