Merge lp:~flacoste/storm/bug-360846-upstream into lp:storm

Proposed by Francis J. Lacoste
Status: Merged
Merged at revision: not available
Proposed branch: lp:~flacoste/storm/bug-360846-upstream
Merge into: lp:storm
Diff against target: 93 lines
To merge this branch: bzr merge lp:~flacoste/storm/bug-360846-upstream
Reviewer Review Type Date Requested Status
James Henstridge Approve
Review via email: mp+6358@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Francis J. Lacoste (flacoste) wrote :

Hi again,

So it seems that the previous fix didn't work in practice. But we found the
root cause (at least from the storm pov): the disconnection was happening in
the tracer code when setting the statement timeout.

So this new branch runs the tracing code through _check_disconnect. That way
any disconnect in the tracer code will trigger a reconnection. I have tests
this time.

One issue pending is what to do with the InterfaceError. That error is raised
when using a connection which has been disconnected. So if everything is fine,
this error is never triggered in practice. But if there is a Storm bug, like
here, the damages are great since that error will never trigger a
reconnection and we'll need to restart app servers. But at the same time,
making InterfaceError reconnects, would hide the underlying storm bug
(something should be run through _check_disconnect, but isn't).

I discussed this fix with Jamu and we couldn't decide on what was the right
thing to do. We thought that a compromise where the error is recovered from,
but at the same time a 'soft oops' is raised would be best. Unfortunately,
storm doesn't use the OOPS system (which isn't a flaw in itself). So I'd like
to hear your opinion on that.

Revision history for this message
James Henstridge (jamesh) wrote :

> Hi again,
>
> So it seems that the previous fix didn't work in practice. But we found the
> root cause (at least from the storm pov): the disconnection was happening in
> the tracer code when setting the statement timeout.

Good catch.

> So this new branch runs the tracing code through _check_disconnect. That way
> any disconnect in the tracer code will trigger a reconnection. I have tests
> this time.

The changes look good to me.

> One issue pending is what to do with the InterfaceError. That error is raised
> when using a connection which has been disconnected. So if everything is fine,
> this error is never triggered in practice. But if there is a Storm bug, like
> here, the damages are great since that error will never trigger a
> reconnection and we'll need to restart app servers. But at the same time,
> making InterfaceError reconnects, would hide the underlying storm bug
> (something should be run through _check_disconnect, but isn't).
>
> I discussed this fix with Jamu and we couldn't decide on what was the right
> thing to do. We thought that a compromise where the error is recovered from,
> but at the same time a 'soft oops' is raised would be best. Unfortunately,
> storm doesn't use the OOPS system (which isn't a flaw in itself). So I'd like
> to hear your opinion on that.

I agree that these sort of errors are likely to keep on occurring in various circumstances (e.g. I think it'd be possible to trigger this with the storm.django module), so we should make it easy for users to handle them.

I think the best option would be to catch these errors and convert them to a subclass of DisconnectionError. For Zope apps, these errors will usually remain uncaught and bubble up until they are passed to the publication's handleError() method where it gets converted to a Retry.

Using a subclass means that existing apps that are checking for DisconnectionError here will continue to work, and apps that want to log these errors can do so here with a full traceback too.

I guess that can be done in a separate branch though: the changes in this branch are worth merging independent of that change.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storm/database.py'
2--- storm/database.py 2008-09-30 11:45:14 +0000
3+++ storm/database.py 2009-05-08 18:20:10 +0000
4@@ -274,8 +274,9 @@
5 @return: The dbapi cursor object, as fetched from L{build_raw_cursor}.
6 """
7 raw_cursor = self.build_raw_cursor()
8- trace("connection_raw_execute", self, raw_cursor,
9- statement, params or ())
10+ self._check_disconnect(
11+ trace, "connection_raw_execute", self, raw_cursor,
12+ statement, params or ())
13 if params:
14 args = (statement, tuple(self.to_database(params)))
15 else:
16@@ -283,12 +284,14 @@
17 try:
18 self._check_disconnect(raw_cursor.execute, *args)
19 except Exception, error:
20- trace("connection_raw_execute_error", self, raw_cursor,
21- statement, params or (), error)
22+ self._check_disconnect(
23+ trace, "connection_raw_execute_error", self, raw_cursor,
24+ statement, params or (), error)
25 raise
26 else:
27- trace("connection_raw_execute_success", self, raw_cursor,
28- statement, params or ())
29+ self._check_disconnect(
30+ trace, "connection_raw_execute_success", self, raw_cursor,
31+ statement, params or ())
32 return raw_cursor
33
34 def _ensure_connected(self):
35
36=== modified file 'tests/database.py'
37--- tests/database.py 2008-04-04 23:03:34 +0000
38+++ tests/database.py 2009-05-08 18:20:10 +0000
39@@ -227,6 +227,54 @@
40 ("ERROR", self.connection, RawCursor,
41 "something", (), error)])
42
43+ def test_tracing_check_disconnect(self):
44+ tracer = FakeTracer()
45+ tracer_mock = self.mocker.patch(tracer)
46+ tracer_mock.connection_raw_execute(ARGS)
47+ self.mocker.throw(DatabaseError('connection closed'))
48+ self.mocker.replay()
49+
50+ install_tracer(tracer_mock)
51+ self.connection.is_disconnection_error = (
52+ lambda exc: 'connection closed' in str(exc))
53+
54+ self.assertRaises(DisconnectionError,
55+ self.connection.execute, "something")
56+
57+ def test_tracing_success_check_disconnect(self):
58+ tracer = FakeTracer()
59+ tracer_mock = self.mocker.patch(tracer)
60+ tracer_mock.connection_raw_execute(ARGS)
61+ tracer_mock.connection_raw_execute_success(ARGS)
62+ self.mocker.throw(DatabaseError('connection closed'))
63+ self.mocker.replay()
64+
65+ install_tracer(tracer_mock)
66+ self.connection.is_disconnection_error = (
67+ lambda exc: 'connection closed' in str(exc))
68+
69+ self.assertRaises(DisconnectionError,
70+ self.connection.execute, "something")
71+
72+ def test_tracing_error_check_disconnect(self):
73+ cursor_mock = self.mocker.patch(RawCursor)
74+ cursor_mock.execute(ARGS)
75+ error = ZeroDivisionError()
76+ self.mocker.throw(error)
77+ tracer = FakeTracer()
78+ tracer_mock = self.mocker.patch(tracer)
79+ tracer_mock.connection_raw_execute(ARGS)
80+ tracer_mock.connection_raw_execute_error(ARGS)
81+ self.mocker.throw(DatabaseError('connection closed'))
82+ self.mocker.replay()
83+
84+ install_tracer(tracer_mock)
85+ self.connection.is_disconnection_error = (
86+ lambda exc: 'connection closed' in str(exc))
87+
88+ self.assertRaises(DisconnectionError,
89+ self.connection.execute, "something")
90+
91 def test_commit(self):
92 self.connection.commit()
93 self.assertEquals(self.executed, ["COMMIT"])

Subscribers

People subscribed via source and target branches

to status/vote changes: