Skip to content

Commit 6fa2c0f

Browse files
shiradyromayalon
authored andcommitted
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> (cherry picked from commit fe4115c)
1 parent 2f21700 commit 6fa2c0f

File tree

5 files changed

+163
-24
lines changed

5 files changed

+163
-24
lines changed

config.js

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

951951
config.NFSF_UPLOAD_STREAM_MEM_THRESHOLD = 8 * 1024 * 1024;
952952

953+
// we want to change our handling related to EACCESS error
954+
config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES = true;
955+
953956
////////////////////////////
954957
// NSFS NON CONTAINERIZED //
955958
////////////////////////////

src/native/fs/fs_napi.cpp

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

773773
/**
774774
* Stat is an fs op
775+
*
776+
* Note: this stat operation contains the system call of open.
777+
* Currently, we use it in list objects, but might want to create a different stat call
778+
* (or add changes inside this) to avoid permission check during list objects
779+
* while we stat each file (to avoid EACCES error)
775780
*/
776781
struct Stat : public FSWorker
777782
{

src/sdk/namespace_fs.js

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -713,10 +713,19 @@ class NamespaceFS {
713713
if (!delimiter && r.common_prefix) {
714714
await process_dir(r.key);
715715
} else {
716-
if (pos < results.length) {
717-
results.splice(pos, 0, r);
718-
} else {
719-
results.push(r);
716+
const entry_path = path.join(this.bucket_path, r.key);
717+
// If entry is outside of bucket, returns stat of symbolic link
718+
const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path));
719+
const stat = await native_fs_utils.stat_if_exists(fs_context, entry_path,
720+
use_lstat, config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES);
721+
if (stat) {
722+
r.stat = stat;
723+
// add the result only if we have the stat information
724+
if (pos < results.length) {
725+
results.splice(pos, 0, r);
726+
} else {
727+
results.push(r);
728+
}
720729
}
721730
if (results.length > limit) {
722731
results.length = limit;
@@ -756,6 +765,11 @@ class NamespaceFS {
756765
await insert_entry_to_results_arr(r);
757766
};
758767

768+
// our current mechanism - list the files and skipping inaccessible directory (invisible in the list).
769+
// We use this check_access in case the directory is not accessible inside a bucket.
770+
// 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.
771+
// We did it outside to avoid undefined values in the cache.
772+
// Note: It is not the same case as a file without permission.
759773
if (!(await this.check_access(fs_context, dir_path))) return;
760774
try {
761775
if (list_versions) {
@@ -877,13 +891,6 @@ class NamespaceFS {
877891

878892
const prefix_dir_key = prefix.slice(0, prefix.lastIndexOf('/') + 1);
879893
await process_dir(prefix_dir_key);
880-
await Promise.all(results.map(async r => {
881-
if (r.common_prefix) return;
882-
const entry_path = path.join(this.bucket_path, r.key);
883-
//If entry is outside of bucket, returns stat of symbolic link
884-
const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path));
885-
r.stat = await nb_native().fs.stat(fs_context, entry_path, { use_lstat });
886-
}));
887894
const res = {
888895
objects: [],
889896
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: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,31 @@ function should_retry_link_unlink(err) {
289289
return should_retry_general || should_retry_gpfs || should_retry_posix;
290290
}
291291

292+
293+
/**
294+
* stat_if_exists execute stat on entry_path and ignores on certain error codes.
295+
* @param {nb.NativeFSContext} fs_context
296+
* @param {string} entry_path
297+
* @param {boolean} use_lstat
298+
* @param {boolean} should_ignore_eacces
299+
* @returns {Promise<nb.NativeFSStats | undefined>}
300+
*/
301+
async function stat_if_exists(fs_context, entry_path, use_lstat, should_ignore_eacces) {
302+
try {
303+
return await nb_native().fs.stat(fs_context, entry_path, { use_lstat });
304+
} catch (err) {
305+
// we might want to expand the error list due to permission/structure
306+
// change (for example: ELOOP, ENAMETOOLONG) or other reason (EPERM) - need to be decided
307+
if ((err.code === 'EACCES' && should_ignore_eacces) ||
308+
err.code === 'ENOENT' || err.code === 'ENOTDIR') {
309+
dbg.log0('stat_if_exists: Could not access file entry_path',
310+
entry_path, 'error code', err.code, ', skipping...');
311+
} else {
312+
throw err;
313+
}
314+
}
315+
}
316+
292317
////////////////////////
293318
/// NON CONTAINERIZED //
294319
////////////////////////
@@ -683,6 +708,7 @@ exports.copy_bytes = copy_bytes;
683708
exports.finally_close_files = finally_close_files;
684709
exports.get_user_by_distinguished_name = get_user_by_distinguished_name;
685710
exports.get_config_files_tmpdir = get_config_files_tmpdir;
711+
exports.stat_if_exists = stat_if_exists;
686712

687713
exports._is_gpfs = _is_gpfs;
688714
exports.safe_move = safe_move;

0 commit comments

Comments
 (0)