tiledb_vfs_open is not reporting error correctly

[robertbindar]

Save error on the context before returning error code.

---

TYPE: BUG
DESC: Save error on the context before returning error code in vfs_open.

[shortcut-integration]

This pull request has been linked to Shortcut Story #33908: tiledb_vfs_open doesn't report error correctly if open fails.

[eric-hughes-tiledb]

This statement should throw rather than manually saving the error. throw_if_not_ok(st) is sufficient.

We're setting up to change error handling away from storing it on the context and returning it. We'll redefine the symbol capi_return_t at that point.

[eric-hughes-tiledb]

Even better would be move the call to open into the constructor of tiledb_vfs_fh_handle_t and throw if the open fails. That'll save a try block and any need to break a handle.

[bekadavis9]

+1

[robertbindar]

@eric-hughes-tiledb @bekadavis9 @ihnorton
Please note the following:

• I moved the open() call in the tiledb_vfs_fh_handle_t constructor as Eric requested
• But I also moved the close() call in the tiledb_vfs_fh_handle_t destructor, this way tiledb_vfs_close and tiledb_vfs_fh_is_closed become noops
• There is a catch though: during closing of a file handle (previously opened in WRITE mode), if no data was written, the close() call creates an empty file. So moving the closing of the file handle in the tiledb_vfs_fh_handle_t and out of the tiledb_vfs_close() api can be considered breaking change. The double catch is that in the documentation of tiledb_vfs_open the exact opposite of that behavior is documented, see it at https://tiledb-inc-tiledb.readthedocs-hosted.com/en/stable/c-api.html#_CPPv415tiledb_vfs_openP12tiledb_ctx_tP12tiledb_vfs_tPKc17tiledb_vfs_mode_tPP15tiledb_vfs_fh_t
  Pasting here for reference: "If the file is closed after being opened, without having written any data to it, the file will not be created. If you wish to create an empty file, use tiledb_vfs_touch instead."
  What's right and what's wrong?

[robertbindar]

Reverted the change in close() behavior so we can fit this release, I'll file a followup for addressing the comment above.

[eric-hughes-tiledb]

so we can fit this release

It's a good decision.

The API design has problem. We shouldn't have a close function but rather a flush one. We ought to have the invariant that "if the handle exists, we have an open handle". As we learned with an analogous situation with groups, we need to separate the return status of an external write from the end-of-life of the resource. (The specific issue was that a C++ destructor can't throw, but the principle is larger.) From the point of view of transaction processing, a resource should in general abort any pending activity at the end of its life. (There are exceptions to this rule, but that's beyond the scope of a small comment.) The purpose of a flush function is to perform any pending activity in advance of end-of-life. With that available, we don't need a close function.

[robertbindar]

Filed https://app.shortcut.com/tiledb-inc/story/33965/tiledb-vfs-close-behavior-is-contradictory-with-regards-to-vfs-docs to track the ideas above.

Edit: Also filed https://app.shortcut.com/tiledb-inc/story/33961/refactor-c-api-vfs-test-unit-capi-vfs-so-that-tests-are-independent-of-each-other because I noticed test cases are not independent.

[eric-hughes-tiledb]

LGTM.

Final code is much cleaner.