Skip to content

protect read-write-extend operations with a lock#1010

Merged
wenyongh merged 2 commits intobytecodealliance:mainfrom
lum1n0us:thread_safe_bh_vector
Mar 23, 2022
Merged

protect read-write-extend operations with a lock#1010
wenyongh merged 2 commits intobytecodealliance:mainfrom
lum1n0us:thread_safe_bh_vector

Conversation

@lum1n0us
Copy link
Contributor

No description provided.

vector->num_elems = 0;

if (use_lock) {
vector->lock = wasm_runtime_malloc(sizeof(korp_mutex));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should check the return value


if (use_lock) {
vector->lock = wasm_runtime_malloc(sizeof(korp_mutex));
os_mutex_init(vector->lock);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should check the return value

#include "bh_vector.h"

#define LOCK(v) \
{ \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had better wrap the code with do { .. } while (0)

@lum1n0us lum1n0us force-pushed the thread_safe_bh_vector branch from 0ae194e to bae0fb2 Compare February 24, 2022 23:04
return false;
}

if (BHT_ERROR == os_mutex_init(vector->lock)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use 0 != os_mutex_init or BHT_OK != os_mutex_init, as os_mutex_init is implemented by various platforms, the return value may be not BHT_ERROR when we failed to init it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it

@lum1n0us lum1n0us force-pushed the thread_safe_bh_vector branch from bae0fb2 to d3e179c Compare March 23, 2022 01:29
- The thread model of _wasm_c_api_ is

- An `wasm_engine_t` instance may only be created once per process
- Every `wasm_store_t` and its objects only be accessed in a single thread
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may only be accessed


- `wasm_engine_new`, `wasm_engine_new_with_config`, `wasm_engine_new_with_args`,
`wasm_engine_delete`should be called in a thread-safe environment. Such
behaviors are not recommend , and please make sure an appropriate calling
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are not recommend , to are not recommended,

behaviors are not recommend , and please make sure an appropriate calling
sequence if it has to be.

- to call `wasm_engine_new` and `wasm_engine_delete` in different threads
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to call means do it in the future, had better change it to call?

sequence if it has to be.

- to call `wasm_engine_new` and `wasm_engine_delete` in different threads
- to call `wasm_engine_new` or `wasm_engine_delete` multiple time in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple times

@lum1n0us lum1n0us force-pushed the thread_safe_bh_vector branch from d3e179c to cfbe7ed Compare March 23, 2022 02:17
@wenyongh wenyongh merged commit 86b79cf into bytecodealliance:main Mar 23, 2022
wenyongh added a commit to wenyongh/wasm-micro-runtime that referenced this pull request Mar 23, 2022
Enable lock for Vector to protect wasm-c-api read/write/extend operations (bytecodealliance#1010)
@lum1n0us lum1n0us deleted the thread_safe_bh_vector branch May 10, 2022 14:44
vickiegpt pushed a commit to vickiegpt/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants