Skip to content

Eager source cleanup is not exception-safe around read_all() in geotiff reader #2322

@brendancol

Description

@brendancol

Describe the bug

In xrspatial/geotiff/_reader.py, _read_to_array constructs a source
(_CloudSource, _FileSource, or _BytesIOSource) at the end of the
local-or-cloud branch, then calls data = src.read_all() on line 187
before entering the try/finally block on line 190 that calls
src.close().

If read_all() raises mid-read (a network blip on a fsspec source, a
transient S3 error, a local I/O failure), the exception propagates out
of the function and src.close() is never called.

Right now _CloudSource.close() is a no-op, so nothing actually leaks.
That makes this latent rather than urgent, but it's fragile. As soon as
the cloud source grows real state (pinned credentials, persistent
fsspec sessions, cached handles, retry counters), every failed read
leaks it.

Expected behavior

src.close() runs even when read_all() raises.

Reproduction sketch

Build a fake source whose read_all() raises, call _read_to_array
on it, and check that src.close() was called. Today it isn't.

Fix direction

Move the try/finally to start right after src is constructed and
put read_all() inside the protected region.

Additional context

The CloudSizeLimitError paths a few lines above already call
src.close() before raising. The eager-read path should inherit the
same guarantee structurally rather than relying on cleanup being a
no-op.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions