passiomatic / coldsweat

Web RSS aggregator and reader compatible with the Fever API
MIT License
145 stars 21 forks source link

Caught exceptions are still logged #52

Closed tewe closed 10 years ago

tewe commented 10 years ago

A client may mark items read multiple times due to bad connectivity.

fever.py line 164 deals with that, but peewee still generates a scary log entry.

I noticed that the code uses exceptions-as-control-flow all over the place. Is this a good idea?

passiomatic commented 10 years ago

fever.py line 164 deals with that, but peewee still generates a scary log entry.

Yes, it is a bit annoying to see that exceptions in the log file. Currently I let Peewee to log the exception by setting level to WARN in init.py:

logging.getLogger("peewee").setLevel(logging.WARN)

One idea to change this could be to silence Peewee logger altogether (or raise its level to CRITICAL) unless Coldsweat loglevel is different from DEBUG.

I noticed that the code uses exceptions-as-control-flow all over the place. Is this a good idea?

That code style is - generally speaking - the Python-way to deal with operations that potentially could not succeed. See for example: Why is it “Easier to ask forgiveness than permission” in python, but not in Java?

tewe commented 10 years ago

I still think asking forgiveness is a bad idea when dealing with something as unforgiving as an RDBMS.

In this case peewee offers .get_or_create rather than .get. But worryingly that doesn't use a transaction, and we'd lose the debug message.

passiomatic commented 10 years ago

I've never used .get_or_create since it is deprecated since version 2.0 of Peewee (http://peewee.readthedocs.org/en/latest/peewee/api.html#Model.get_or_create).

Anyway, in the specific example we are discussing about a corner case. You are right when you write we are "...dealing with something unforgiving as an RDBMS" and that's exactly why I let RDBMS to decide if it can accept or not the new Read record. The RDBMS is more rigid than the outside world when it comes to enforce integrity.

tewe commented 10 years ago

The problem with letting the database decide is that, once it decides you made an error, you've left normalcy. SQL logs might fill up. Shared hosting might freeze you. People get paged out of bed.

And peewee does nothing to adjust that to what might be pythonic, it also treats an error as an error, and logs it.

So your code has the two layers it depends on hyperventilating all the time because it's too lazy to do basic sanity checks.

Sorry if that came across strong, in this case it's a really small issue.

passiomatic commented 10 years ago

Just to recap, here there's where I'm trapping IntegrityError in the code:

fetcher.py

fever.py

frontend.py

models.py

In fever and frontend modules I already take extra steps to ensure a myriad of IntegrityError exceptions aren't raised when marking every thing as read, so the issue should be very limited.

As anticipated I've made the Peewee and Requests loggers less verbose while in production (level != DEBUG). This should alleviate the pain :)

Let me know if it works for you.

passiomatic commented 10 years ago

Ah, now understands why Peewee started out of the blue to log errors that previously ignored: https://github.com/coleifer/peewee/issues/254

Looks like other people are experiencing this issue. I'll keep an eye on it.

passiomatic commented 10 years ago

For the record: next Peewee release won't log errors — https://github.com/coleifer/peewee/commit/69db85238935654eb0f69174038a7f8016e8b0f0