[CDRIVER-2002] Given mongoc_gridfs_file_readv() returning -1 (failure), mongoc_gridfs_file_error() doesn't indicate error condition Created: 25/Jan/17  Updated: 03/May/17  Resolved: 28/Mar/17

Status: Closed
Project: C Driver
Component/s: GridFS
Affects Version/s: 1.5.3
Fix Version/s: 1.7.0

Type: Bug Priority: Minor - P4
Reporter: Arseny Vakhrushev Assignee: A. Jesse Jiryu Davis
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: File libmongoc3.c    
Issue Links:
Related
related to CDRIVER-2006 mongoc_gridfs_find_one_with_opts(): t... Closed
related to CDRIVER-895 Review GridFS errno behavior/document... Closed

 Description   

There is a case when mongoc_gridfs_file_readv() returns -1 (failure), but the subsequent call to mongoc_gridfs_file_error() returns false indicating that there is no actual error.

This seems to contradict with what the manual states about mongoc_gridfs_file_readv():

Returns the number of bytes read, or -1 on failure. Use mongoc_gridfs_file_error to retrieve error details.

Interestingly, but for mongoc_gridfs_file_writev(), it states that errno will be set:

Returns the number of bytes written or -1 upon error and errno is set.

Not sure how potential database failures will be propagated via errno, but at least I can't blame this one for now albeit it looks a bit inconsistent.

Please see the example below.

#include <mongoc.h>
 
int main() {
	mongoc_client_t *client;
	mongoc_gridfs_t *gridfs;
	mongoc_gridfs_file_t *file;
	bson_error_t error;
	ssize_t n;
	char buf[] = "Some content for a new file";
	mongoc_iovec_t iov = { .iov_base = buf, .iov_len = sizeof buf };
 
	mongoc_init();
 
	client = mongoc_client_new("mongodb://127.0.0.1");
	BSON_ASSERT(client);
	gridfs = mongoc_client_get_gridfs(client, "test-gridfs", 0, &error);
	BSON_ASSERT(gridfs);
 
	
	file = mongoc_gridfs_create_file(gridfs, 0); // Create file
	BSON_ASSERT(file);
	BSON_ASSERT(mongoc_gridfs_file_writev(file, &iov, 1, 0) == sizeof buf); // Write contents
	BSON_ASSERT(mongoc_gridfs_file_seek(file, 0, SEEK_SET) == 0); // Set position to 0
	BSON_ASSERT(mongoc_gridfs_file_save(file)); // Save file
	BSON_ASSERT(mongoc_gridfs_file_remove(file, &error)); // Remove file
 
	// Now the issue ....
 
	// mongoc_gridfs_file_readv() fails as expected
	BSON_ASSERT(mongoc_gridfs_file_readv(file, &iov, 1, sizeof buf, 0) == -1);
 
	// Now it's time to check for the actual error, right?
	// But the following will fail ...
	BSON_ASSERT(mongoc_gridfs_file_error(file, &error));
 
	// Similarly, mongoc_gridfs_file_writev() fails as expected
	BSON_ASSERT(mongoc_gridfs_file_writev(file, &iov, 1, 0) == -1);
 
	// The following will fail if uncommented. Maybe that's expected though.
	// BSON_ASSERT(mongoc_gridfs_file_error(file, &error));
 
 
	mongoc_gridfs_file_destroy(file);
	mongoc_gridfs_destroy(gridfs);
	mongoc_client_destroy(client);
	mongoc_cleanup();
	return 0;
}

The output:

$ ./a.out
libmongoc3.c:34 main(): precondition failed: mongoc_gridfs_file_error(file, &error)



 Comments   
Comment by Githook User [ 28/Mar/17 ]

Author:

{u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}

Message: CDRIVER-2002 gridfs_readv sets error on missing file
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/0f5651ed10b5ad564411a010c8bab06d43ddfd62

Comment by Arseny Vakhrushev [ 29/Jan/17 ]

That one piece of code looks like the culprit indeed.

But please note, the same underlying low-level condition (absence of a chunk) may lead to a high-level EOF condition (returned value is 0 instead of -1). I achieved that by playing with seeking back and forth through the contents of an invalid file. It doesn't imply anything. It just might be a hint that something else is failing...

Comment by A. Jesse Jiryu Davis [ 25/Jan/17 ]

Thanks, I think the problem is here, where we check if the mongoc_cursor_next has returned true (we got the first chunk) or false (we didn't):

https://github.com/mongodb/mongo-c-driver/blob/7f1dd3630d6bb4b8d5db4c2b865c328acd170012/src/mongoc/mongoc-gridfs-file.c#L812-L812

The code assumes that if it returns false, then cursor->error is set, so we can copy it to file->error. But if the cursor simply retrieves no chunks, it returns false and has no error info.

The original "errno"-based error reporting for some functions is absurd and inconsistent but it's what we have right now. There is a ticket CDRIVER-925 to review the current API and check for bugs and for documentation errors.

In the future we may choose to implement a second, better-thought-out GridFS API for the C Driver.

Generated at Wed Feb 07 21:13:51 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.