DEV Community

DuckDev
DuckDev

Posted on

The Python import that silently disabled a feature for months

I was reading an open-source instrument-control library to add docstrings — a boring job, deliberately. Boring jobs make you read code you'd otherwise skim.

About two hundred lines into one function I found a caching feature that had never worked. Not "worked badly." Never executed successfully, not once, and never left a trace of failing.

Here's the shape of it, reduced to the part that matters:

def get_instrument(resource_address, driver_type="GENERIC"):

    if resource_address == "AUTO":
        import json                      # <-- line 93
        ...
        # auto-discovery path, uses json happily

    # ... 150 lines of other logic ...

    # Update cache with successful manual connection
    # to enable future AUTO discovery
    if resource_address != "AUTO":
        try:
            with open(cache_file, "r") as f:
                cached_resources = json.load(f)     # <-- line 297
            ...
        except Exception:
            pass
Enter fullscreen mode Exit fullscreen mode

Read it once and it looks fine. json gets imported, json gets used.

It isn't fine, and the reason is a scoping rule that's easy to know and still miss in the wild.

import is an assignment

import json doesn't just make a module available. It binds the name json in the current scope. Inside a function, that's a local binding — exactly like json = <module>.

And Python decides whether a name is local at compile time, for the whole function body. Not line by line. If a name is assigned anywhere in a function, it is local everywhere in that function, including on lines that execute before the assignment.

So json on line 297 does not resolve to a global. It resolves to the local that line 93 would have created — and line 93 only runs when resource_address == "AUTO", while line 297 only runs when it isn't.

The two lines are on mutually exclusive branches. The binding never exists when the use happens.

The minimal version

def f(addr):
    if addr == "AUTO":
        import json          # makes `json` a LOCAL of f()
        return "auto path"
    try:
        return "cached: " + json.dumps({"a": 1})
    except Exception as e:
        return f"swallowed -> {type(e).__name__}: {e}"
Enter fullscreen mode Exit fullscreen mode
>>> f("AUTO")
'auto path'

>>> f("TCPIP::1.2.3.4::INSTR")
"swallowed -> UnboundLocalError: cannot access local variable 'json'
 where it is not associated with a value"
Enter fullscreen mode Exit fullscreen mode

UnboundLocalError, not NameError. That distinction is the tell: Python knew json was local, it just had nothing in it yet.

The second bug is the one that hid it

An UnboundLocalError is loud. It has a traceback. Someone would have fixed this in an afternoon.

Except the whole block was wrapped in:

except Exception:
    pass
Enter fullscreen mode Exit fullscreen mode

UnboundLocalError subclasses NameError, which subclasses Exception. So the bare handler ate it, every time, in silence.

That's the actual failure. The scoping mistake was a five-minute fix that nobody got the chance to make, because the code that was supposed to surface it was configured to discard everything.

The comment above the block said "Update cache with successful manual connection to enable future AUTO discovery." It did nothing of the kind. Manual connections were never cached, so auto-discovery never learned from them, so it kept doing a full scan every time — the exact cost the cache existed to avoid.

Worth noting: the same file elsewhere gets this right:

except (IOError, OSError, json.JSONDecodeError):
    pass
Enter fullscreen mode Exit fullscreen mode

Specific. That handler would have let the UnboundLocalError through on day one.

What I'd take from it

Put imports at module scope unless you have a concrete reason not to. The usual reasons are real — breaking a circular import, deferring an expensive or optional dependency. "It's only used in this branch" is not one of them. The cost of a top-level import is a dictionary lookup.

If you must import inside a function, put it at the top of the function, not inside a conditional. Then the binding exists on every path that can reach a use.

except Exception: pass is not error handling. It's error deletion. It converts a loud bug into a feature that quietly doesn't exist. If you're catching around file I/O, catch (IOError, OSError). If you don't know what can be raised, that's the thing to find out before writing the handler.

And the general one: a silent failure inside a broad except is invisible to tests that only assert on outcomes. Nothing crashed. Nothing logged. The function returned the right object. The only symptom was a performance optimisation that never optimised anything — and you don't notice a cache miss you were always going to have.

The bug wasn't hard. Finding it needed someone to read the code slowly with no agenda, which is a thing that basically never gets scheduled.


Reported upstream with a runnable reproduction; the fix is moving one line. If you want the real thing rather than the reduced version, it's abduznik/instrumation#135.

Top comments (0)