SWIFT-294, SWIFT-394 Fix memory leaks #237
Conversation
| } | ||
|
|
||
| internal init(fromPointer pointer: BSONPointer) { | ||
| internal init(copying pointer: BSONPointer) { |
There was a problem hiding this comment.
Consider adding documentation here. I think you tend to do it at the call site, but it's probably useful to future readers why these are distinguished
There was a problem hiding this comment.
added docs for all three DocumentStorage initializers.
| * of `bsonData`, so the caller is responsible for freeing the original | ||
| * memory. | ||
| * Initializes a `Document` from a pointer to a `bson_t` by making a copy of the | ||
| * data. The caller is responsible for freeing the original `bson_t`. |
There was a problem hiding this comment.
I don't think this is always correct. For instance, in the APM code above we copy the pointer, and are not responsible for freeing the original bson_t
There was a problem hiding this comment.
good point, I updated it to just say that whoever owns the bson_t is responsible for freeing it.
| } | ||
|
|
||
| internal init(stealing pointer: MutableBSONPointer) { | ||
| self.pointer = pointer |
There was a problem hiding this comment.
what if self.pointer is already set?
There was a problem hiding this comment.
since this is an initializer, I don't think that is possible.
There was a problem hiding this comment.
that's a... really good point 😄 I'm like a hammer looking for nails over here
Sources/MongoSwift/MongoCursor.swift
Outdated
| fatalError("mongoc_cursor_next returned true, but document is nil") | ||
| } | ||
|
|
||
| // we have to copy because libmongoc owns the pointer. |
patrickfreed
left a comment
patrickfreed
left a comment
There was a problem hiding this comment.
Everything, looks good, just one question on some removed code
| * | ||
| * - Returns: a new `Document` | ||
| */ | ||
| internal init(copying pointer: BSONPointer) { |
There was a problem hiding this comment.
I really like the copying / stealing labels; it makes it super clear what is going on without needing to refer to the docs. 👍
This fixes several memory leaks in the driver. Kudos to @valeriomazzeo for pointing these out in #236.
I've done the following:
Document(fromPointer:)andDocumentStorage(fromPointer:)toDocument(copying:)andDocumentStorage(copying:)to make it clearer what their behavior isDocumentStorage(stealing:)initializer that takes over responsibility for the providedbson_t, and aDocument(stealing:)initializer that calls through to itfromPointerinitializers were used to call through to the correct new methodsI also fixed a leak of the
mongoc_uri_tin theMongoClientinitializer, and a leak we'd previously identified in SDAM monitoring (SWIFT-294).