Skip to content

Commit fe4115c

Browse files
committed
NSFS | Fix Bug | Race between list object and delete object during operation
1. Replace the function stat_ignore_eacces with stat_if_exists and add ignore of ENOENT and ENOTDIR (in addition to EACCES with a flag of config.EACCES_IGNORE_ENTRY). 2. Call stat on the entry_path one time and save is instead of calling it in concurrency so that we can be sure that the value was not deleted from the result array. Move the stat call before results.push(r) and results.splice(pos, 0, r) as they both add a result to the array. 3. Add a comment before the check_access, see also in PR description of Added check_access() call to list objects #6576. This change is continuing PR #8751 by adding a case to ignore a stat failure entry. Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
1 parent cd7c240 commit fe4115c

File tree

5 files changed

+160
-36
lines changed

5 files changed

+160
-36
lines changed

config.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -913,6 +913,9 @@ config.ANONYMOUS_ACCOUNT_NAME = 'anonymous';
913913

914914
config.NFSF_UPLOAD_STREAM_MEM_THRESHOLD = 8 * 1024 * 1024;
915915

916+
// we want to change our handling related to EACCESS error
917+
config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES = true;
918+
916919
////////////////////////////
917920
// NSFS NON CONTAINERIZED //
918921
////////////////////////////

src/native/fs/fs_napi.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -752,6 +752,11 @@ struct FSWrapWorker : public FSWorker
752752

753753
/**
754754
* Stat is an fs op
755+
*
756+
* Note: this stat operation contains the system call of open.
757+
* Currently, we use it in list objects, but might want to create a different stat call
758+
* (or add changes inside this) to avoid permission check during list objects
759+
* while we stat each file (to avoid EACCES error)
755760
*/
756761
struct Stat : public FSWorker
757762
{

src/sdk/namespace_fs.js

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -718,11 +718,17 @@ class NamespaceFS {
718718
if (!delimiter && r.common_prefix) {
719719
await process_dir(r.key);
720720
} else {
721-
if (pos < results.length) {
722-
results.splice(pos, 0, r);
723-
} else {
724-
const stat = await native_fs_utils.stat_ignore_eacces(this.bucket_path, r, fs_context);
725-
if (stat) {
721+
const entry_path = path.join(this.bucket_path, r.key);
722+
// If entry is outside of bucket, returns stat of symbolic link
723+
const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path));
724+
const stat = await native_fs_utils.stat_if_exists(fs_context, entry_path,
725+
use_lstat, config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES);
726+
if (stat) {
727+
r.stat = stat;
728+
// add the result only if we have the stat information
729+
if (pos < results.length) {
730+
results.splice(pos, 0, r);
731+
} else {
726732
results.push(r);
727733
}
728734
}
@@ -764,6 +770,11 @@ class NamespaceFS {
764770
await insert_entry_to_results_arr(r);
765771
};
766772

773+
// our current mechanism - list the files and skipping inaccessible directory (invisible in the list).
774+
// We use this check_access in case the directory is not accessible inside a bucket.
775+
// In a directory if we don’t have access to the directory, we want to skip the directory and its sub directories from the list.
776+
// We did it outside to avoid undefined values in the cache.
777+
// Note: It is not the same case as a file without permission.
767778
if (!(await this.check_access(fs_context, dir_path))) return;
768779
try {
769780
if (list_versions) {
@@ -885,13 +896,6 @@ class NamespaceFS {
885896

886897
const prefix_dir_key = prefix.slice(0, prefix.lastIndexOf('/') + 1);
887898
await process_dir(prefix_dir_key);
888-
await Promise.all(results.map(async r => {
889-
if (r.common_prefix) return;
890-
const entry_path = path.join(this.bucket_path, r.key);
891-
//If entry is outside of bucket, returns stat of symbolic link
892-
const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path));
893-
r.stat = await nb_native().fs.stat(fs_context, entry_path, { use_lstat });
894-
}));
895899
const res = {
896900
objects: [],
897901
common_prefixes: [],

src/test/unit_tests/jest_tests/test_nsfs_concurrency.test.js

Lines changed: 111 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ const P = require('../../../util/promise');
66
const fs_utils = require('../../../util/fs_utils');
77
const NamespaceFS = require('../../../sdk/namespace_fs');
88
const buffer_utils = require('../../../util/buffer_utils');
9-
const { TMP_PATH } = require('../../system_tests/test_utils');
9+
const { TMP_PATH, TEST_TIMEOUT } = require('../../system_tests/test_utils');
1010
const { crypto_random_string } = require('../../../util/string_utils');
1111
const endpoint_stats_collector = require('../../../sdk/endpoint_stats_collector');
1212

@@ -27,18 +27,20 @@ function make_dummy_object_sdk(nsfs_config, uid, gid) {
2727
}
2828

2929
const DUMMY_OBJECT_SDK = make_dummy_object_sdk(true);
30+
31+
const tmp_fs_path = path.join(TMP_PATH, 'test_nsfs_concurrency');
32+
33+
const nsfs = new NamespaceFS({
34+
bucket_path: tmp_fs_path,
35+
bucket_id: '1',
36+
namespace_resource_id: undefined,
37+
access_mode: undefined,
38+
versioning: 'DISABLED',
39+
force_md5_etag: false,
40+
stats: endpoint_stats_collector.instance(),
41+
});
42+
3043
describe('test nsfs concurrency', () => {
31-
const tmp_fs_path = path.join(TMP_PATH, 'test_nsfs_concurrency');
32-
33-
const nsfs = new NamespaceFS({
34-
bucket_path: tmp_fs_path,
35-
bucket_id: '1',
36-
namespace_resource_id: undefined,
37-
access_mode: undefined,
38-
versioning: 'DISABLED',
39-
force_md5_etag: false,
40-
stats: endpoint_stats_collector.instance(),
41-
});
4244

4345
beforeEach(async () => {
4446
await fs_utils.create_fresh_path(tmp_fs_path);
@@ -68,5 +70,101 @@ describe('test nsfs concurrency', () => {
6870
}
6971
await P.delay(5000);
7072
expect(res_etags).toHaveLength(15);
71-
}, 6000);
73+
}, TEST_TIMEOUT);
74+
75+
test_list_and_delete({
76+
test_name: 'list objects and delete an object during it - random deletion',
77+
bucket_name: 'bucket1',
78+
num_of_objects_to_upload: 5,
79+
expected_num_of_object_in_list: 4,
80+
key_to_delete: `my-key-${random_integer(1, 5)}`,
81+
iterations: 5,
82+
});
83+
84+
test_list_and_delete({
85+
test_name: 'list objects and delete an object during it - delete the last object',
86+
bucket_name: 'bucket2',
87+
num_of_objects_to_upload: 1000,
88+
expected_num_of_object_in_list: 999,
89+
key_to_delete: `my-key-1000`,
90+
iterations: 1
91+
});
7292
});
93+
94+
/**
95+
* @param {{
96+
* test_name: string,
97+
* bucket_name: string,
98+
* num_of_objects_to_upload: number,
99+
* expected_num_of_object_in_list: number,
100+
* key_to_delete?: string,
101+
* iterations?: number,
102+
* }} params
103+
*/
104+
function test_list_and_delete({
105+
test_name,
106+
bucket_name,
107+
num_of_objects_to_upload,
108+
expected_num_of_object_in_list,
109+
key_to_delete,
110+
iterations = 1
111+
}) {
112+
113+
it(test_name, async () => {
114+
await _upload_objects(bucket_name, num_of_objects_to_upload);
115+
if (key_to_delete === undefined) {
116+
key_to_delete = `my-key-${random_integer(1, expected_num_of_object_in_list)}`;
117+
console.log('test_list_and_delete: key_to_delete', key_to_delete);
118+
}
119+
120+
console.log(`test_list_and_delete: ${test_name} ${bucket_name} num_of_objects_to_upload: ${num_of_objects_to_upload},
121+
expected_num_of_object_in_list ${expected_num_of_object_in_list} key_to_delete ${key_to_delete}`);
122+
123+
for (let i = 0; i < iterations; ++i) {
124+
nsfs.list_objects({ bucket: bucket_name }, DUMMY_OBJECT_SDK)
125+
.catch(err => {
126+
console.log('error during list_objects', err);
127+
throw err;
128+
}).then(res => {
129+
console.log('list was successful');
130+
});
131+
nsfs.delete_object({ bucket: bucket_name, key: key_to_delete }, DUMMY_OBJECT_SDK)
132+
.catch(err => {
133+
console.log('delete_object got an error', err);
134+
throw err;
135+
}).then(res => {
136+
console.log('delete_object during list objects was successful');
137+
});
138+
await P.delay(5000);
139+
// up to this point if it was successful, the race between the delete object and list object went fine.
140+
}
141+
}, TEST_TIMEOUT);
142+
}
143+
144+
/**
145+
* _upload_objects uploads number_of_versions of objects in bucket
146+
* note: this function is not concurrent, it's a helper function for preparing a bucket with a couple of objects
147+
* @param {string} bucket
148+
* @param {number} number_of_objects
149+
*/
150+
async function _upload_objects(bucket, number_of_objects) {
151+
const keys_names = [];
152+
for (let i = 0; i < number_of_objects; i++) {
153+
const key_name = `my-key-${i + 1}`;
154+
const random_data = Buffer.from(String(crypto_random_string(7)));
155+
const body = buffer_utils.buffer_to_read_stream(random_data);
156+
await nsfs.upload_object({ bucket: bucket, key: key_name, source_stream: body }, DUMMY_OBJECT_SDK);
157+
keys_names.push(key_name);
158+
}
159+
return keys_names;
160+
}
161+
162+
/**
163+
* randomInteger between min (included) and max (included)
164+
* // copied from: https://stackoverflow.com/questions/4959975/generate-random-number-between-two-numbers-in-javascript
165+
* @param {number} min
166+
* @param {number} max
167+
*/
168+
function random_integer(min, max) {
169+
return Math.floor(Math.random() * (max - min + 1)) + min;
170+
}

src/util/native_fs_utils.js

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,30 @@ async function stat_ignore_enoent(fs_context, file_path) {
303303
}
304304
}
305305

306+
/**
307+
* stat_if_exists execute stat on entry_path and ignores on certain error codes.
308+
* @param {nb.NativeFSContext} fs_context
309+
* @param {string} entry_path
310+
* @param {boolean} use_lstat
311+
* @param {boolean} should_ignore_eacces
312+
* @returns {Promise<nb.NativeFSStats | undefined>}
313+
*/
314+
async function stat_if_exists(fs_context, entry_path, use_lstat, should_ignore_eacces) {
315+
try {
316+
return await nb_native().fs.stat(fs_context, entry_path, { use_lstat });
317+
} catch (err) {
318+
// we might want to expand the error list due to permission/structure
319+
// change (for example: ELOOP, ENAMETOOLONG) or other reason (EPERM) - need to be decided
320+
if ((err.code === 'EACCES' && should_ignore_eacces) ||
321+
err.code === 'ENOENT' || err.code === 'ENOTDIR') {
322+
dbg.log0('stat_if_exists: Could not access file entry_path',
323+
entry_path, 'error code', err.code, ', skipping...');
324+
} else {
325+
throw err;
326+
}
327+
}
328+
}
329+
306330
////////////////////////
307331
/// NON CONTAINERIZED //
308332
////////////////////////
@@ -688,16 +712,6 @@ function translate_error_codes(err, entity) {
688712
return err;
689713
}
690714

691-
async function stat_ignore_eacces(bucket_path, result, fs_context) {
692-
const entry_path = path.join(bucket_path, result.key);
693-
try {
694-
return await nb_native().fs.stat(fs_context, entry_path);
695-
} catch (err) {
696-
if (err.code !== 'EACCES') throw err;
697-
dbg.log0('NamespaceFS: check_stats_for_object : couldnt access file entry_path', entry_path, ", skipping...");
698-
}
699-
}
700-
701715
exports.get_umasked_mode = get_umasked_mode;
702716
exports._make_path_dirs = _make_path_dirs;
703717
exports._create_path = _create_path;
@@ -708,6 +722,7 @@ exports.finally_close_files = finally_close_files;
708722
exports.get_user_by_distinguished_name = get_user_by_distinguished_name;
709723
exports.get_config_files_tmpdir = get_config_files_tmpdir;
710724
exports.stat_ignore_enoent = stat_ignore_enoent;
725+
exports.stat_if_exists = stat_if_exists;
711726

712727
exports._is_gpfs = _is_gpfs;
713728
exports.safe_move = safe_move;
@@ -739,4 +754,3 @@ exports.get_bucket_tmpdir_full_path = get_bucket_tmpdir_full_path;
739754
exports.get_bucket_tmpdir_name = get_bucket_tmpdir_name;
740755
exports.entity_enum = entity_enum;
741756
exports.translate_error_codes = translate_error_codes;
742-
exports.stat_ignore_eacces = stat_ignore_eacces;

0 commit comments

Comments
 (0)