Fix: allocate C memory for MDB_val in readonly Txn #29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use C.malloc to allocate memory for MDB_val in readonly Txn. CGo does not allow using new() for this. Fixes #28.
shane-ns1
reviewed
Dec 7, 2023
Change the C.mdb_filehandle_t declaration to use var instead of new() to clarify that we are not retaining a pointer to anything. This is related to #28, but in this case no issue, because we are not assiging a pointer to Go allocated memory to a C value. Functionally this does not change anything. Go decides if values are stack allocated or heap allocated based on if they escape, not based on the syntax.
wojas
changed the title
shane-ns1
reviewed
Dec 7, 2023
shane-ns1
reviewed
Dec 7, 2023
| @@ -943,6 +943,9 @@ func TestTxn_Reset_readonly_C_free(t *testing.T) { | |||
|
|
|||
| // Since this is a readonly transaction, the global Env key/val cannot be | |||
| // reused and new C memory must be allocated. | |||
| if txn.key == env.ckey || txn.val == env.cval { | |||
| t.Error("env.ckey and cval must not be used in a readonly Txn") | |||
Co-authored-by: Shane Kerr <[email protected]>
ahouene
approved these changes
Dec 7, 2023
|
I've run my code with this patch for a while and didn't see the problem anymore. Thanks! Now I will also remove the goroutine stuff, which would probably have fixed it anyway somehow. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use C.malloc to allocate memory for MDB_val in readonly Txn.
CGo does not allow using new() for this. This can lead to an "cgo argument has Go pointer to unpinned Go pointer" CGo error.
Fixes #28.
TODO:
Also fix the FD() usage of new() if needed.Not needed, but changed syntax for clarity.@fiatjaf: does this fix the error you encountered?