regebro / tzlocal

A Python module that tries to figure out what your local timezone is
MIT License
184 stars 58 forks source link

Improved error handling for more than IOError #99

Closed revansSZ closed 3 years ago

revansSZ commented 3 years ago

The code that checks /etc/sysconfig/clock errors out for Centos 8. The file appears to be binary, but the file open operation is for text.

The exception code meant to handle this only handles IOError, but not other error types. I adjusted to include except Exception as e: -- In other words, continue on any exception not just IOError.

This allows the code to proceed and instead inspect /etc/localtime which returns properly.

agronholm commented 3 years ago

I don't think such a broad except clause should be used. What is the actual raised exception in this case?

regebro commented 3 years ago

I can not find any support for the claim that CentOS 8 has a binary /etc/sysconfig/clock. In CentOS 5 & 6 it was a text file, in 7 & 8 it seems to not exist. I think this is a misconfiguration of your system.

I'm guessing you get a unicode error? That seems to be the correct behavior in this case.

revansSZ commented 3 years ago

Hmm, I see the deviation, the vendor of this particular Centos 8 server has the following: /etc/sysconfig/clock is a symbolic link to /etc/localtime which is binary. Correct we get a unicode decode error, but the Except IOError does not catch it to continue, instead we have a hard failure.

Centos 8 vanilla doesn't have a /etc/sysconfig/clock file at all, in preference of using timedatectl

file -s clock clock: symbolic link to /etc/localtime

Compare that to centos 6 which has: file -s clock clock: ASCII text

Although this is a vendor quirk trying to create a needless symlink from /etc/sysconfig/clock -> /etc/localtime. My argument is tzlocal should have no reason to fail to catch an exception at this point, unless you can think of a reason where it should fatally fail on decoding a file.

It should just continue on evaluating the next file path which it does with except Exception as e:

We could alternatively do except (IOError, UnicodeDecodeError) as e: -- and then continue from there.

regebro commented 3 years ago

Well, that's a bug with the vendor, but I'm not opposed to trapping the UnicodeDecodeError there. We could also just open all files as binary, and change the regexps to be binary.

revansSZ commented 3 years ago

Let me know if the updated pull is acceptable.

agronholm commented 3 years ago

The new comment is useless – if there's a block for except (IOError, UnicodeError):, what information does it add when you say that IOError and UnicodeError are handled there? This is obvious for any Python programmer. A comment should explain why these specific exceptions are ignored.

revansSZ commented 3 years ago

The new comment is useless – if there's a block for except (IOError, UnicodeError):, what information does it add when you say that IOError and UnicodeError are handled there? This is obvious for any Python programmer. A comment should explain why these specific exceptions are ignored.

Fair enough, I'll adjust the comment. If however you guys are draconian with comment usage, there are several such examples of those. Such as:

//The fact that the try proceeded without exception implies that "it works" try: tz = ZoneInfo(tzenv)

That worked, so we return this:

    return tz

//The below is indeed implied, but an explicit comment can be nice except IOError:

File doesn't exist or is a directory

At any rate, here is the adjusted comment.

UnicodeDecode handles edge case where /etc/sysconfig/clock is symlink to /etc/localtime

regebro commented 3 years ago

Thanks!