Skip to content

Commit e94c5bd

Browse files
authored
94 cache files can be corrupted by interrupted writes (#100)
* disable logger warning * Atomic writes * test write_atomic * move cache tests into single file * test logging * clean-up
1 parent 8240af6 commit e94c5bd

7 files changed

Lines changed: 202 additions & 72 deletions

File tree

hapiclient/cache.py

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -125,28 +125,21 @@ def meta_cache_write(meta, SERVER, DATASET, opts):
125125
"""Write metadata to JSON and PKL cache files."""
126126

127127
import os
128-
import json
129-
import pickle
130128

131129
from hapiclient.log import log
130+
from hapiclient.util import write_atomic
132131

133132
if not opts["cache"]:
134133
return
135134

136135
paths = meta_cache_paths(SERVER, DATASET, opts['cachedir'])
137136
fnamejson, fnamepkl = paths['json'], paths['pkl']
138137

139-
server_dir = cachedir(opts["cachedir"], SERVER)
140-
os.makedirs(server_dir, exist_ok=True)
141-
142138
log('Writing %s ' % os.path.basename(fnamejson))
143-
with open(fnamejson, 'w') as f:
144-
json.dump(meta, f, indent=4)
139+
write_atomic(fnamejson, meta)
145140

146141
log('Writing %s ' % os.path.basename(fnamepkl))
147-
with open(fnamepkl, 'wb') as f:
148-
# protocol=2 used for Python 2.7 compatibility.
149-
pickle.dump(meta, f, protocol=2)
142+
write_atomic(fnamepkl, meta)
150143

151144

152145
def data_cache_paths(SERVER, DATASET, PARAMETERS, START, STOP, cachedir):
@@ -215,11 +208,9 @@ def data_cache_write(data_result, meta, SERVER, DATASET, PARAMETERS, START, STOP
215208
"""
216209

217210
import os
218-
import pickle
219-
import warnings
220-
import numpy as np
221211

222212
from hapiclient.log import log
213+
from hapiclient.util import write_atomic
223214

224215
data_paths = data_cache_paths(SERVER, DATASET, PARAMETERS, START, STOP, opts['cachedir'])
225216
fnamecsv, fnamebin, fnamenpy, fnamepklx = data_paths['csv'], data_paths['bin'], data_paths['npy'], data_paths['pkl']
@@ -236,20 +227,8 @@ def data_cache_write(data_result, meta, SERVER, DATASET, PARAMETERS, START, STOP
236227
# Need to return after meta is updated.
237228
return
238229

239-
server_dir = cachedir(opts["cachedir"], SERVER)
240-
os.makedirs(server_dir, exist_ok=True)
241-
242230
log('Writing %s' % os.path.basename(fnamepklx))
243-
with open(fnamepklx, 'wb') as f:
244-
pickle.dump(meta, f, protocol=2)
231+
write_atomic(fnamepklx, meta)
245232

246233
log('Writing %s' % os.path.basename(fnamenpy))
247-
with warnings.catch_warnings():
248-
# Ignore warning that occurs when saving Unicode data.
249-
kwargs = {
250-
'message': r"Stored array in format 3\.0.*",
251-
'category': UserWarning,
252-
'module': r"numpy\.lib\.format"
253-
}
254-
warnings.filterwarnings("ignore", **kwargs)
255-
np.save(fnamenpy, data_result)
234+
write_atomic(fnamenpy, data_result)

hapiclient/data.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ def data(SERVER, DATASET, PARAMETERS, START, STOP, opts):
2222
log('STOP was given as None. Getting stopDate for dataset.')
2323
meta = info(SERVER, DATASET, None, opts)
2424
STOP = meta['stopDate']
25-
log('Using STOP = {STOP}')
25+
log(f'Using STOP = {STOP}')
2626

2727
tic_totalTime = time.time()
2828

hapiclient/util.py

Lines changed: 71 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,6 @@ def error(msg, debug=False):
197197
from inspect import stack
198198
from os import path
199199

200-
debug = False
201200
if pythonshell() != 'shell':
202201
try:
203202
from IPython.core.interactiveshell import InteractiveShell
@@ -214,15 +213,15 @@ def prefix():
214213
import platform
215214
prefix = "\033[0;31mHAPIError:\033[0m "
216215
if platform.system() == 'Windows' and pythonshell() == 'shell':
217-
prefix = "HAPIError: "
216+
prefix = "HAPIError: "
218217

219218
return prefix
220219

221220
def exception_handler_ipython(self, exc_tuple=None,
222221
filename=None, tb_offset=None,
223222
exception_only=False,
224223
running_compiled_code=False):
225-
224+
226225
exception = sys.exc_info()
227226
if not debug and exception[0].__name__ == "HAPIError":
228227
sys.stderr.write(prefix() + str(exception[1]))
@@ -347,26 +346,19 @@ def get_full_class_name(obj):
347346

348347

349348
def urlretrieve(url, fname):
350-
"""Download URL to file
349+
"""Download URL to file atomically.
351350
352351
res = urlretrieve(url, fname)
353352
"""
354353

355-
import os
356-
import shutil
357-
358-
dirname = os.path.dirname(fname)
359-
if not os.path.exists(dirname):
360-
os.makedirs(dirname)
354+
log('Writing')
355+
log(' %s' % url)
356+
log('to')
357+
log(' %s' % fname)
358+
res = urlopen(url)
359+
write_atomic(fname, res)
361360

362-
with open(fname, 'wb') as out:
363-
res = urlopen(url)
364-
log('Writing')
365-
log('%s' % url)
366-
log('to')
367-
log('%s' % os.path.basename(fname))
368-
shutil.copyfileobj(res, out)
369-
return res
361+
return res
370362

371363

372364
def subset_meta(meta, params):
@@ -394,7 +386,7 @@ def subset_meta(meta, params):
394386
pa = [meta['parameters'][0]] # First parameter is always the time parameter
395387

396388
params_reordered = [] # Re-ordered params
397-
# If time parameter explicity requested, put it first in params_reordered.
389+
# If time parameter explicitly requested, put it first in params_reordered.
398390
if meta['parameters'][0]['name'] in p:
399391
params_reordered = [meta['parameters'][0]['name']]
400392

@@ -454,3 +446,63 @@ def missing_length(meta, opts):
454446
return True
455447

456448
return False
449+
450+
451+
def write_atomic(path, data):
452+
453+
import os
454+
import json
455+
import pickle
456+
import pathlib
457+
import secrets
458+
import warnings
459+
460+
import numpy
461+
462+
path = pathlib.Path(path)
463+
path.parent.mkdir(parents=True, exist_ok=True)
464+
tmp_ext = f".{secrets.token_hex(3)}.tmp"
465+
tmp_path = path.with_suffix(path.suffix + tmp_ext)
466+
467+
try:
468+
469+
if path.suffix == '.json':
470+
with tmp_path.open('w') as f:
471+
json.dump(data, f, indent=2)
472+
473+
if path.suffix == '.pkl':
474+
with tmp_path.open('wb') as f:
475+
pickle.dump(data, f, protocol=2)
476+
477+
if path.suffix == '.npy':
478+
with warnings.catch_warnings():
479+
# Ignore warning that occurs when saving Unicode data.
480+
kwargs = {
481+
'message': r"Stored array in format 3\.0.*",
482+
'category': UserWarning,
483+
'module': r"numpy\.lib\.format"
484+
}
485+
warnings.filterwarnings("ignore", **kwargs)
486+
with tmp_path.open('wb') as f:
487+
numpy.save(f, data)
488+
489+
if path.suffix in ('.bin', '.csv'):
490+
with tmp_path.open('wb') as f:
491+
if isinstance(data, (bytes, bytearray)):
492+
f.write(data)
493+
else:
494+
# Assume a file-like / streaming response object.
495+
import shutil
496+
shutil.copyfileobj(data, f)
497+
498+
try:
499+
os.replace(tmp_path, path)
500+
except Exception as e:
501+
warning(f"Failed to rename cache file from {tmp_path} to {path}: {e}")
502+
503+
except Exception as e:
504+
warning(f"Failed to write cache file {tmp_path}: {e}")
505+
try:
506+
tmp_path.unlink()
507+
except OSError:
508+
warning(f"Failed to remove temporary cache file {tmp_path}")

test/test_cache.py

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
# See ../README.md for instructions on running tests.
2+
import shutil
3+
4+
from hapiclient.hapi import hapi
5+
6+
from util import compare
7+
8+
from util.get_logger import get_logger
9+
logger = get_logger(__name__)
10+
11+
kwargs = {
12+
'cache': False,
13+
'usecache': False,
14+
'cachedir': '/tmp/hapi-data',
15+
'logging': False
16+
}
17+
18+
server = 'http://hapi-server.org/servers/TestData2.0/hapi'
19+
dataset = 'dataset1'
20+
start = '1970-01-01'
21+
stop = '1970-01-01T00:00:03'
22+
23+
def test_cache_short():
24+
25+
# Compare read with empty cache with read with hot cache and usecache=True
26+
27+
opts = {**kwargs, 'cache': True}
28+
29+
opts['usecache'] = False
30+
shutil.rmtree(opts['cachedir'], ignore_errors=True)
31+
data, _ = hapi(server, dataset, 'scalarint,vectorint', start, stop, **opts)
32+
33+
opts['usecache'] = True
34+
data2, _ = hapi(server, dataset, 'scalarint,vectorint', start, stop, **opts)
35+
36+
assert compare.equal(data, data2)
37+
38+
39+
def test_cache_error():
40+
41+
from unittest.mock import patch
42+
43+
import io
44+
import contextlib
45+
import pathlib
46+
import tempfile
47+
from hapiclient.util import write_atomic
48+
49+
def assert_warns(fn, expected):
50+
buf = io.StringIO()
51+
with contextlib.redirect_stderr(buf):
52+
result = fn()
53+
print(buf.getvalue())
54+
msg = f"Expected '{expected}' in stderr: {buf.getvalue()!r}"
55+
assert expected in buf.getvalue(), msg
56+
return result
57+
58+
# Direct calls to write_atomic()
59+
with tempfile.TemporaryDirectory() as tmpdir:
60+
path = pathlib.Path(tmpdir) / 'test.json'
61+
data = {'key': 'value'}
62+
63+
def call_write_atomic():
64+
write_atomic(str(path), data)
65+
66+
# Simulate write failure
67+
with patch('json.dump', side_effect=OSError('No space left on device')):
68+
assert_warns(call_write_atomic, 'Failed to write cache file')
69+
assert not path.exists(), 'File should not exist after write failure'
70+
71+
# Simulate os.replace failure after successful write
72+
with patch('os.replace', side_effect=OSError('Permission denied')):
73+
assert_warns(call_write_atomic, 'Failed to rename cache file from')
74+
assert not path.exists(), 'File should not exist after rename failure'
75+
76+
# Simulate write failure AND unlink failure
77+
patch1 = patch('json.dump', side_effect=OSError('No space left on device'))
78+
patch2 = patch('pathlib.Path.unlink', side_effect=OSError('Permission denied'))
79+
with patch1, patch2:
80+
assert_warns(call_write_atomic, 'Failed to remove temporary cache file')
81+
82+
# Simulate successful write
83+
write_atomic(str(path), data)
84+
assert path.exists(), 'File should exist after successful write'
85+
86+
87+
# Indirect calls to write_atomic()
88+
dataset = 'dataset1'
89+
start = '1970-01-01'
90+
stop = '1970-01-01T00:00:03'
91+
92+
opts = {**kwargs, 'cache': True, 'usecache': False}
93+
94+
def call_hapi():
95+
data, meta = hapi(server, dataset, 'scalarint,vectorint', start, stop, **opts)
96+
return data, meta
97+
98+
def assert_data_valid(data):
99+
msg = hapi(server, dataset, 'scalarint,vectorint', start, stop, **opts)
100+
assert data['Time'][0] == b'1970-01-01T00:00:00.000Z', msg
101+
102+
with patch('pickle.dump', side_effect=OSError('No space left on device')):
103+
data, _ = assert_warns(call_hapi, 'Failed to write cache file')
104+
assert_data_valid(data)
105+
106+
with patch('os.replace', side_effect=OSError('Permission denied')):
107+
data, _ = assert_warns(call_hapi, 'Failed to rename cache file from')
108+
assert_data_valid(data)
109+
110+
p1 = patch('pickle.dump', side_effect=OSError('No space left on device'))
111+
p2 = patch('pathlib.Path.unlink', side_effect=OSError('Permission denied'))
112+
with p1, p2:
113+
data, _ = assert_warns(call_hapi, 'Failed to remove temporary cache file')
114+
assert_data_valid(data)
115+
116+
117+
if __name__ == '__main__':
118+
test_cache_short()
119+
test_cache_error()

test/test_hapi_data_requests.py

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -97,25 +97,6 @@ def test_server(version):
9797
pytest.fail("test_server('%s') raised: %s" % (version, e))
9898

9999

100-
def test_cache_short():
101-
102-
# Compare read with empty cache with read with hot cache and usecache=True
103-
dataset = 'dataset1'
104-
start = '1970-01-01'
105-
stop = '1970-01-01T00:00:03'
106-
107-
opts = {**kwargs, 'cache': True}
108-
109-
opts['usecache'] = False
110-
shutil.rmtree(opts['cachedir'], ignore_errors=True)
111-
data, meta = hapi(server, dataset, 'scalarint,vectorint', start, stop, **opts)
112-
113-
opts['usecache'] = True
114-
data2, meta2 = hapi(server, dataset, 'scalarint,vectorint', start, stop, **opts)
115-
116-
assert compare.equal(data, data2)
117-
118-
119100
def test_subset_short():
120101

121102
dataset = 'dataset1'
@@ -212,6 +193,7 @@ def test_unicode():
212193
assert compare.read(server, dataset, parameter, run, opts.copy(), logger=logger)
213194
assert compare.cache(server, dataset, parameter, opts.copy(), logger=logger)
214195

196+
215197
def test_empty_response():
216198
from hapiclient import hapi
217199

@@ -249,6 +231,5 @@ def test_empty_response():
249231
test_reader_timing_long()
250232
test_all_test_servers()
251233
test_subset_short()
252-
test_cache_short()
253234
test_request2path()
254235
test_unicode()

0 commit comments

Comments
 (0)