Code review comment for lp:~mzanetti/reminders-app/rework-error-handling

Revision history for this message
Michael Zanetti (mzanetti) wrote :

> 1. In notes.h, I believe you'll want to include QQmlParserStatus in a
> Q_INTERFACES macro.

Hmm... Why is that?

> 2. I see you're reworking some of the connection error exception handling. I
> get the following core dump every so often, I wonder if it's related to an
> error that isn't being handled.
>
> terminate called after throwing an instance of
> 'apache::thrift::transport::TSSLException'
> what(): SSL_write: errno = 110
> Aborted (core dumped)

Interesting. I intentionally only caught the exceptions I could see myself in order to learn about libthrift's behavior. I haven't ever gotten this one. I'll add catching that one and also all the others that can happen in theory as I've seen enough of libthrift by now.

Still, if you could tell me how to reproduce this I could check if there is something else I'm doing wrong and try to prevent it from happening instead of just catching and ignoring.

>
> 3. Overall the changes to job handling and error handling look good--is there
> any particular error path that's being fixed that I might be able to try to
> reproduce? It's a bit hard to try to verify an overall rework of exception
> handling.

Nope, not really. The main reason for this commit is that I didn't want to add the try {} catch() block around every single call to the evernote SDK. So this now offers one place to catch them all and still pass the error code back to the job in a Qt style manner (As Qt intentionally does not use exceptions by design).

« Back to merge proposal