fix: free msg_env unconditionally #22
Conversation
| @@ -86,23 +86,16 @@ erlfdb_future_cb(FDBFuture* fdb_future, void* data) | |||
| cancelled = future->cancelled; | |||
| enif_mutex_unlock(future->cancel_lock); | |||
|
|
|||
| enif_mutex_lock(future->msg_lock); | |||
There was a problem hiding this comment.
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.
| @@ -86,23 +86,16 @@ erlfdb_future_cb(FDBFuture* fdb_future, void* data) | |||
| cancelled = future->cancelled; | |||
| enif_mutex_unlock(future->cancel_lock); | |||
There was a problem hiding this comment.
Stupid question, do we released the lock too early?
image two processes are cancelling the future if it can happen?
There was a problem hiding this comment.
fdb allows double cancelling; it only forbids double destroy
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
future->cancelled is independently set from two different nifs; so, formally, we need synchronization
No description provided.