[CDRIVER-3811] mongoc_gridfs_create_file_from_stream() has inconsistent error handling semantics Created: 29/Oct/20 Updated: 03/Jun/21 Resolved: 03/Jun/21 |
|
| Status: | Closed |
| Project: | C Driver |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Michael Mlsna | Assignee: | Unassigned |
| Resolution: | Won't Fix | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
Although the documentation for mongoc_gridfs_create_file_from_stream() does not clearly state whether the user is responsible for cleaning up the mongoc_stream_t on success/failure, from reading the source code, examples, and tests in the repo, it seems like the intended contract is:
However, callers cannot simply call mongoc_stream_destroy() if the function returns NULL, because the stream is only valid on certain error paths, not all error paths. Looking at https://github.com/mongodb/mongo-c-driver/blob/1.17.1/src/libmongoc/src/mongoc/mongoc-stream.c#L78:
The three NULL return paths have been labeled. For #1 and #2, the mongoc_stream_t will be valid after the call returns, but for #3 it will have already been deallocated by mongoc_stream_failed(). Given there is no way to programmatically distinguish the error return paths, the only safe behavior currently is to avoid calling mongoc_stream_destroy() on error, which leads to resource leaks. |
| Comments |
| Comment by Rachelle Palmer [ 03/Jun/21 ] |
|
We've decided not to fix this issue as it is for our deprecated API. |
| Comment by Michael Mlsna [ 03/Nov/20 ] |
|
I appreciate the quick turnaround for triaging this. And thank you for pointing out the mongoc_gridfs_bucket_t API, somehow I completely missed the big deprecation warning on the API Reference site |
| Comment by Kevin Albertson [ 03/Nov/20 ] |
|
Hi mmlsna@vailsys.com, thank you for the detailed report! I agree this appears to be a bug. At a glance, I am thinking of these solutions to make it unambiguous: Solution A: Solution B: Solution A seems more appealing. It seems like a less harmful backwards break of behavior than solution B. We will fix this as soon as we can, though we also welcome pull requests against the project if you are interested in contributing. Also note, mongoc_gridfs_t and the associated API has been superseded by the mongoc_gridfs_bucket_t API (documented here), which adheres to the cross-driver GridFS specification. If possible, we recommend switching to that API. |