Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3244 defect closed fixed (fixed)

runInteraction exceptions are swallowed if rollback fails

Reported by: therve Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: itamarst Branch: branches/adbapi-errors-3244
(github, coverage, patch, buildbot, log)
Author: therve

Description (last modified by therve)

In twisted.enterprise.adbapi.ConnectionPool._runInteraction, if the interaction provided fails, and that rollback fails for some reasons, you get a ConnectionLost error instead of the original exception raised by the interaction.

In general, all the errors raised should be logged (close and reopen seem to be wrong here).

Attachments (1)

fix-adbapi.diff (1.9 KB) - added by itamarst 8 years ago.
Sketch of fix

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by therve

  • Description modified (diff)

Changed 8 years ago by itamarst

Sketch of fix

comment:2 Changed 8 years ago by itamarst

The attached patch makes sure no except: clauses mask errors, and that if rollback raises error the original error is not masked. It's not tested, and arguably it's not backwards compatible in the case where rollback fails (since you'll get original error raised instead of ConnectionLost).

comment:3 Changed 8 years ago by therve

  • author set to therve
  • Branch set to branches/adbapi-errors-3244

(In [24297]) Branching to 'adbapi-errors-3244'

comment:4 Changed 8 years ago by therve

(In [24298]) Add tests to itamar's fix. Refs #3244.

comment:5 Changed 8 years ago by therve

  • Keywords review added
  • Owner therve deleted

Please review!

comment:6 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

I think all the log.err calls should include a string describing the context in which the error happened (eg, the one in rollback might just say "Rollback failed"). This makes it easier to read logs with tracebacks in them.

There's a bunch of trailing whitespace in adbapi.py, might be good to clean all that up.

For some reason, a lot of twisted.test.test_adbapi fails for me with this branch. Attaching a log. If you can't easily reproduce this, I could probably try to investigate.

comment:7 Changed 8 years ago by therve

  • Keywords review added
  • Owner therve deleted

I think it should be better.

comment:8 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Did you disagree with the first two points?

comment:9 Changed 8 years ago by therve

I'm tired.

comment:10 Changed 8 years ago by therve

  • Keywords review added
  • Owner therve deleted

OK, now review may be handled.

comment:11 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

The parameter names to log.err are pretty stupid. I think the best spelling for this is log.err(None, "..."), rather than explicitly naming the _why parameter. What do you think?

Other than that, looks good. Merge when you're ready.

comment:12 Changed 8 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [24348]) Merge adbapi-errors-3244

Author: therve Reviewer: exarkun Fixes #3244

Fix several exception handling in adbapi, to log every errors and raise original ones in case of rollback.

comment:13 Changed 8 years ago by exarkun

This was basically a duplicate of #1754.

comment:14 Changed 5 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.