Bug #5074

[SDKs] CLI client and Ruby SDK should recover from (and avoid creating) unreadable discovery doc cache files.

Added by Tom Clegg over 5 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
SDKs
Target version:
Start date:
01/27/2015
Due date:
% Done:

100%

Estimated time:
(Total: 1.00 h)
Story points:
0.5

Description

The current code creates a race condition while writing a file. It should create a temp file, write it, flush/close it, and rename it into place. If anything fails, it should delete the temp file.

Additionally, if a cache file exists (and is new enough, etc.) but is unreadable or unparseable, the SDK crashes. Instead, it should print a warning, then retrieve the discovery document from the server and continue.

See source:sdk/cli/bin/arv#L55 and source:sdk/ruby/lib/arvados.rb#L108

Bonus points: de-duplicate this copy-and-pasted code!


Subtasks

Task #5091: Review 5074-ruby-sdk-discovery-cache-wipResolvedPeter Amstutz

Associated revisions

Revision 0fb26747
Added by Brett Smith over 5 years ago

Merge branch '5074-ruby-sdk-discovery-cache-wip'

Closes #5074, #5091.

History

#1 Updated by Tom Clegg over 5 years ago

  • Subject changed from [SDKs] Ruby SDK should recover from (and avoid creating) unreadable discovery doc cache files. to [SDKs] CLI client and Ruby SDK should recover from (and avoid creating) unreadable discovery doc cache files.
  • Description updated (diff)
  • Category set to SDKs

#2 Updated by Brett Smith over 5 years ago

  • Status changed from New to In Progress
  • Assigned To set to Brett Smith
  • Target version changed from Bug Triage to 2015-01-28 Sprint

#3 Updated by Peter Amstutz over 5 years ago

The code looks good. I appreciate that you moved the logic to the Ruby SDK.

So I intentionally corrupted the cached discovery document and got this error:

Warning: error reading cached discovery document /home/peter/.cache/arvados/discovery-86e60ccc4da91c726aa80dfb73fa9cbf.json: 795: 
unexpected token at ':"discovery#restDescription","discoveryVersion":"v1","id":"arvados:v1","name":"arvados","version":"v1","revision":"20131114",
"source_version":"1fae5d5","generatedAt":"2015-01-28T18:52:17+00:00","title":"Arvados API","description":"The API to interact with 
Arvados.","documentationLink":"http://doc.arvados.org/api/index.html","protocol":"rest","baseUrl":"https://qr1hi.arvadosapi.com/arvados/v1/",
... (and so forth for 1000s lines spewing the entire discover doc)

Consider not reporting an error in this case, because it is nonfatal and the user probably can't do anything with the information.

Does the Python discovery document cache code have the same robustness that the Ruby SDK will have with this branch?

Python appears to use a different caching scheme than Ruby, which means the discovery document gets stored twice?

#4 Updated by Brett Smith over 5 years ago

Peter Amstutz wrote:

Consider not reporting an error in this case, because it is nonfatal and the user probably can't do anything with the information.

Yeah, with 100-line error messages that's definitely not going to fly. Updated in d59deccc.

Does the Python discovery document cache code have the same robustness that the Ruby SDK will have with this branch?

The Python SDK lets httplib2 handle caching. I haven't dug into its source but I think we should trust them to deal with it robustly. They have to handle users a lot more demanding than us.

Thanks.

#5 Updated by Peter Amstutz over 5 years ago

LGTM

#6 Updated by Brett Smith over 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:0fb26747fa229d6b19ec911b907259a8e84acd83.

Also available in: Atom PDF