client_python
client_python copied to clipboard
Fix `write_to_textfile` leaves back temp files on errors
Fix #1044
Looks in good to me.
The only two things I'd comment are:
- Catching
Exceptionwon’t e.g. catchKeyboardInterrupt... but probably this doesn’t matter, since I guess there’s no clean-up in that case anyway (e.g. when metrics would be only half-written or so). - At least in principle
os.remove(tmppath)could raise an exception, too. ... One could catch that with anothertryblock and, if caught, set the__cause__of that exception to the original. But probably overkill.
Looks in good to me.
The only two things I'd comment are:
- Catching
Exceptionwon’t e.g. catchKeyboardInterrupt... but probably this doesn’t matter, since I guess there’s no clean-up in that case anyway (e.g. when metrics would be only half-written or so).- At least in principle
os.remove(tmppath)could raise an exception, too. ... One could catch that with anothertryblock and, if caught, set the__cause__of that exception to the original. But probably overkill.
Thanks.
Regarding your first point, I believe the KeyboardInterrupt exception would only occur when triggered manually by the user.I could add handling for this exception, but I'm not sure if it’s necessary for prometheus_client. Perhaps @csmarchbanks could provide some insights.
As for the second point, if the os.remove method throws an exception, the error stack trace is quite clear, as shown below:
Traceback (most recent call last):
File "/WorkSpace/venv/lib/python3.8/site-packages/prometheus_client/exposition.py", line 378, in write_to_textfile
os.rename(tmppath, path)
FileNotFoundError: [Errno 2] No such file or directory: '.80904.140704537089984' -> ''
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "x.py", line 6, in <module>
write_to_textfile('', registry)
File "/WorkSpace/venv/lib/python3.8/site-packages/prometheus_client/exposition.py", line 381, in write_to_textfile
os.remove(1/0)
ZeroDivisionError: division by zero
Therefore, I think avoiding nested exception handling code might make it more elegant.