Story #7697

[SDKs] Improve socket error handling in PySDK

Added by Brett Smith almost 4 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Target version:
Start date:
10/29/2015
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

Description

History: we added brute-force socket error retry handling in #7587. We want to improve it to use the same rules we're using in the Go SDK in #5538. Modify _intercept_http_request to do the following:

  • If we're about to do a POST or DELETE, and (as far as we know) the http client's socket has been idle for more than 30 seconds, close its socket. (httplib2 will automatically open a new one to make the request.) It might be hard to tell how long the socket has been idle: "we haven't used the http client for 30 seconds, and the socket is idle right now" might be a good enough approximation.
  • Retry BadStatusLine and socket.error the same way as we currently do, but only if the original request was idempotent (i.e., HEAD, GET, or PUT).

Subtasks

Task #7850: Avoid re-using old sockets for non-retryable requestsResolvedTom Clegg

Task #7794: Review 7697-socket-retryResolvedTom Clegg

Associated revisions

Revision 96a3d13e
Added by Tom Clegg over 3 years ago

Merge branch '7697-socket-retry' closes #7697

History

#1 Updated by Brett Smith almost 4 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg almost 4 years ago

  • Description updated (diff)

#3 Updated by Brett Smith almost 4 years ago

  • Story points set to 1.0

#4 Updated by Brett Smith almost 4 years ago

  • Target version set to Arvados Future Sprints

#5 Updated by Tom Clegg almost 4 years ago

  • Description updated (diff)

#6 Updated by Brett Smith almost 4 years ago

  • Target version changed from Arvados Future Sprints to 2015-12-02 sprint

#7 Updated by Brett Smith almost 4 years ago

  • Assigned To set to Tom Clegg

#8 Updated by Tom Clegg over 3 years ago

  • Status changed from New to In Progress

#9 Updated by Tom Clegg over 3 years ago

7697-socket-retry @ 54fc73b

#10 Updated by Brett Smith over 3 years ago

54fc73b just has one small bug that should get fixed before merge: except httplib.BadStatusLine, httplib.HTTPException: should just say except httplib.HTTPException:. What you wrote will catch httplib.BadStatusLine exceptions and bind any exception caught to httplib.HTTPException. BadStatusLine inherits from HTTPException, so we can just catch that.

Beyond that, I just have small readability suggestions, none of which are necessary for merge:

  • The import Queue added to test_api.py seems unused.
  • Two blank lines before the RetryREST definition would be nicely PEP 8-compliant.
  • self.api._http.orig_http_request.side_effect = lambda x, *a, **k: self.request_success I think can just be self.api._http.orig_http_request.return_value = self.request_success. side_effect takes precedence over return_value, so test methods that override that will still work as intended.
  • You can DRY up _test_connection_close by changing self.api._http.connections = {str(i): mock_conns[i] for i in range(2)} to self.api._http.connections = {str(i): c for i, c in enumerate(mock_conns)}.

Thanks.

#11 Updated by Tom Clegg over 3 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:96a3d13e88c67f6beef17877f15d97a03c63b525.

Also available in: Atom PDF