Skip to content

Commit 3e038b0

Browse files
authored
Merge pull request #8809 from shirady/nsfs-list-race-delete
NSFS | Fix Bug | Race Between List Object and Delete Object
2 parents cd7c240 + fe4115c commit 3e038b0

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)