Avoid possible GC allocations in MongoCursor.~this #2794
Conversation
This adds a check to the destructor to avoid running into an InvalidMemoryOperationError and instead outputs a more descriptive error message without crashing. This also swaps back the order of operations in killCursors() so that no connection to the server gets allocated by the call if the cursor has already been killed.
mongodb/vibe/db/mongo/cursor.d
Outdated
| // leaked to the GC | ||
| if(GC.inFinalizer) { | ||
| logError("MongoCursor instance that has not been fully processed leaked to the GC!"); | ||
| try throw new Exception(""); |
There was a problem hiding this comment.
this is an allocation, is this even reliable? Might need to check the ABI docs here
There was a problem hiding this comment.
Oops, you are right of course, that won't work, I didn't really think there. Unfortunate that we don't have @nogc stack trace code in DRuntime. I do have an implementation for a different project, maybe it makes sense to publish that on code.dlang.org.
There was a problem hiding this comment.
so I'm not even sure if the definition is exactly not to allocate in a destructor, since it's not listed in the spec: https://dlang.org/spec/class.html#destructors
however it says not to use child data - unsure how it's supposed to work with cyclic things
There was a problem hiding this comment.
The reality with the GC is that finalizers (or struct destructors in that situation) are only (safely) usable for very simple things (calling free() or close()). It's also not just child data, but also any GC allocated memory that poses a problem (i.e. data referenced by static variables). I'm not sure if or where it's mentioned in the spec, but the implementation will error out for any GC action you invoke from within a finalizer call.
While some of this could definitely improved in the GC, I think the only realistic way to avoid issues like this is to make eventcore and the relevant parts of vibe-core fully @nogc.
BTW, another serious shortcoming with the current GC is that you also may only do thread-safe things, so even something like glDeleteTextures() is not an option, and since finalizers are running in parallel to application code, I bet that there are many race-conditions hidden in real-world finalizers.
This adds a check to the destructor to avoid running into an InvalidMemoryOperationError and instead outputs a more descriptive error message without crashing. This also swaps back the order of operations in killCursors() so that no connection to the server gets allocated by the call if the cursor has already been killed.
Fixes #2793.