Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: free msg_env unconditionally #22

Merged
merged 1 commit into from
Apr 12, 2024
Merged

fix: free msg_env unconditionally #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions c_src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,23 +86,16 @@ erlfdb_future_cb(FDBFuture* fdb_future, void* data)
cancelled = future->cancelled;
enif_mutex_unlock(future->cancel_lock);
Copy link

@qzhuyan qzhuyan Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Stupid question, do we released the lock too early?
image two processes are cancelling the future if it can happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fdb allows double cancelling; it only forbids double destroy

Copy link

Choose a reason for hiding this comment

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

If FDB ensures the callback is executed exact once as the cancel could be triggered by FDB or by NIF.
and the deref of future NIF resource is only done (for NIF part) in the callback https://github.com/emqx/couchdb-erlfdb/pull/22/files#diff-ceadfa165520612e5b6b1a255f7680bcaa9e2a67bf47677f6884fd1db3cb03f5L110 .
Then I think the future->cancel_lock is not needed at all?

Copy link

Choose a reason for hiding this comment

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

it is unrelated to this PR though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

future->cancelled is independently set from two different nifs; so, formally, we need synchronization


enif_mutex_lock(future->msg_lock);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The lock seems useless—it saves only from concurrent erlfdb_future_cb invocations, which the fdb driver should prevent.
But if it somehow happens, then we will anyway run into problems calling enif_release_resource(future) twice.


if(future->msg_env != NULL) {

if(!cancelled) {
proc_env = (ErlNifEnv*) erl_drv_tsd_get(future->future_proc_env_key);
msg = T2(future->msg_env, future->msg_ref, ATOM_ready);
enif_send(proc_env, &(future->pid), future->msg_env, msg);
} else {
enif_free_env(future->msg_env);
}

enif_free_env(future->msg_env);
future->msg_env = NULL;
}

enif_mutex_unlock(future->msg_lock);

// We're now done with this future which means we need
// to release our handle to it. See erlfdb_create_future
// for more on why this happens here.
Expand Down Expand Up @@ -132,7 +125,6 @@ erlfdb_create_future(ErlNifEnv* env, FDBFuture* future, ErlFDBFutureGetter gette
enif_self(env, &(f->pid));
f->msg_env = enif_alloc_env();
f->msg_ref = enif_make_copy(f->msg_env, ref);
f->msg_lock = enif_mutex_create("fdb:future_msg_lock");

f->cancelled = false;
f->cancel_lock = enif_mutex_create("fdb:future_cancel_lock");
Expand Down
4 changes: 0 additions & 4 deletions c_src/resources.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,6 @@ erlfdb_future_dtor(ErlNifEnv* env, void* obj)
if(f->cancel_lock != NULL) {
enif_mutex_destroy(f->cancel_lock);
}

if(f->msg_lock != NULL) {
enif_mutex_destroy(f->msg_lock);
}
}


Expand Down
1 change: 0 additions & 1 deletion c_src/resources.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ struct _ErlFDBFuture
ErlNifPid pid;
ErlNifEnv* msg_env;
ERL_NIF_TERM msg_ref;
ErlNifMutex* msg_lock;
ErlDrvTSDKey future_proc_env_key;
bool cancelled;
ErlNifMutex* cancel_lock;
Expand Down