4 ms·
> The biggest offender here is the bare except: pass clause. Never use these. Suppressing exceptions is simply dangerous. This doesn't apply if you're using ex
by amyjess 11y ago
> The biggest offender here is the bare except: pass clause. Never use these. Suppressing exceptions is simply dangerous.
This doesn't apply if you're using exceptions as control flow, which is idiomatic and recommended Python behavior.
Let's say you want to make sure some file doesn't exist before you start doing something. Specifically, you want to wipe it out if it exists, and you don't care if it doesn't exist. You do:
if os.path.exists(temp_file):
os.remove(temp_file)
This isn't idiomatic Python behavior. Instead, you want to use exceptions:
try:
os.remove(temp_file)
except OSError:
pass
Mind you, an actual bare "except:" (without specifying an exception type) is still bad, because that'll catch things you don't want being caught, like a SystemExit or KeyboardInterrupt, but it's perfectly acceptable to silently pass in this case.
- teddyh 11y agoNote that FileNotFoundError is preferred in modern Python instead of OSError. In case of old Python, one should narrow the catching to cover only the wanted case: import os, errno try: os.remove(temp_file) except OSError as e: if e.errno != errno.ENOENT: raise
- revelation 11y agoI can see why this is the preferred idiom.
- jbrownbridge 11y agoI never silently pass on exceptions any more as I've been bitten by way too many edge cases. You could imagine the case where your example is run by a different user to the owner of temp_file and fails to remove it due to permission issues. In my opinion you should at least log some sort of message here.
- to3m 11y agoThis still doesn't solve the problem, though, because you still have the race condition that the exception case is supposed to be fixing. There's probably no good solution to this in this case.
- viraptor 11y agoDid you mean that: try: os.remove(temp_file) except OSError: pass Has a race condition? Care to explain?
- to3m 11y agoSuppose you have the if-then case. You might end up with the file on disk anyway: maybe os.path.exists says false, and it skips the remove, and so some other process sneaks in and creates the file behind your back. By the time you go to do whatever it is that requires this file not to exist, that file is there again. Suppose then you decide to go for the try-except version. You still have the same problem. At some point between os.remove, and whatever you're going to do, this other process could leap in and create the file. Some types of race condition can be solved by simply trying the operation required, rather than checking first and then trying only if it seems necessary, but I don't think this situation is one of them.
- viraptor 11y agoI see what you mean. I wouldn't call it a race condition in that code though. It may be a race condition in the system, but this code fragment on its own is race free - if there's a file, it will be removed. The rest of the system is not present and we can only see the correct-or-not way of file removal. (raising exception or not) Otherwise we'll end up with "a = 1" being a race condition, because it may happen to map to a device memory used by many applications.
- shoyer 11y agoEven better (with Python 3.5 or newer): from contextlib import suppress with suppress(OSError): os.remove(temp_file) More generally, context managers are the right way to refactor repeated try/except/finally clauses.
- amyjess 11y agoWell, I have yet another reason to migrate from 2 to 3 now.
- btilly 11y agoYour example proves his point. Suppose that permissions for the directory that the file is in got screwed up. (Or you're working in a new environment and they weren't set up right.) Your version gives no indication that anything went wrong. The version you dislike gives a helpful error message that says exactly what went wrong. That error message can save a LOT of debugging time. Please make sure it is there.
- jdreaver 11y agoHowever, the first version has a race condition. The file might not exist between the if-statement check and the call to os.remove.
- btilly 11y agoThat sort of issue is why the best practice is to use careful routines like https://docs.python.org/2/library/tempfile.html https://docs.python.org/2/library/tempfile.html for temp files. But in practice on a trusted server, this kind of race condition doesn't actually happen. And if you have someone untrusted on your server, you already have a bigger problem than what you're fixing by getting the race condition right. By contrast the kind of permission problem that I mentioned is common, and I've spent too many hours debugging such issues in code other people wrote. (Ever tried to run old code on a new server that someone else tried to set up? You'd be amazed how many hidden dependencies will turn up...) That is why I consider the race condition to be less important than the exception swallowing anti-pattern.