Skip to content

Fix error handling bugs in url.py makedirs and download_url#2207

Open
Mr-Neutr0n wants to merge 1 commit intoRUCAIBox:masterfrom
Mr-Neutr0n:fix/url-makedirs-error-handling
Open

Fix error handling bugs in url.py makedirs and download_url#2207
Mr-Neutr0n wants to merge 1 commit intoRUCAIBox:masterfrom
Mr-Neutr0n:fix/url-makedirs-error-handling

Conversation

@Mr-Neutr0n
Copy link

Summary

Fixes three related error handling bugs in recbole/utils/url.py:

1. makedirs() silently swallows real OS errors

The except clause has an inverted condition:

except OSError as e:
    if e.errno != errno.EEXIST and osp.isdir(path):
        raise e

This means when errno != EEXIST (e.g., PermissionError) and the path doesn't exist as a directory, the error is silently swallowed. The function returns successfully but the directory was never created, causing confusing failures downstream.

Fix: Replace with os.makedirs(exist_ok=True), which is the correct Python 3 approach and handles all edge cases properly.

2. download_url() bare except: catches KeyboardInterrupt

The bare except: clause catches KeyboardInterrupt and SystemExit, making it difficult to interrupt a long download with Ctrl+C — the user gets a generic RuntimeError("Stopped downloading due to interruption.") instead of a clean exit, and the original traceback is lost.

Fix: Use except Exception: and re-raise the original exception with raise to preserve the traceback.

3. download_url() leaks the URL connection

The ur.urlopen(url) response is never closed, leaking socket/file descriptors. This matters when download_url is called repeatedly or when an exception occurs mid-download.

Fix: Wrap the download logic in try/finally to ensure data.close() is always called.

Test plan

  • Verify makedirs works for new directories
  • Verify makedirs is a no-op for existing directories
  • Verify makedirs raises on permission errors instead of silently failing
  • Verify Ctrl+C during download propagates KeyboardInterrupt cleanly
  • Verify download errors preserve the original exception traceback
  • Verify URL connections are closed after download (success or failure)

- Fix makedirs(): the except clause had an inverted condition
  (e.errno != errno.EEXIST and osp.isdir(path)) that silently
  swallowed real OS errors like PermissionError when the path did
  not exist. Replace with os.makedirs(exist_ok=True) which is the
  correct Python 3 approach and handles all edge cases properly.

- Fix download_url(): replace bare except clause with
  except Exception to allow KeyboardInterrupt and SystemExit to
  propagate correctly. Re-raise the original exception instead of
  replacing it with a generic RuntimeError to preserve the
  traceback for debugging.

- Fix resource leak: ensure the URL connection opened by
  ur.urlopen() is always closed via try/finally, preventing
  socket/file descriptor leaks on both success and failure paths.
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.

1 participant