-
Notifications
You must be signed in to change notification settings - Fork 12
Implement exposing/enforcing coordinator for request #299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3925fa0
to
afed812
Compare
@muzarski Please fill the pre-review checklist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the change in the last commit.
#[unsafe(no_mangle)] | ||
pub unsafe extern "C" fn testing_future_get_host( | ||
future_raw: CassBorrowedSharedPtr<CassFuture, CConst>, | ||
host: *mut *mut c_char, | ||
host_length: *mut size_t, | ||
) { | ||
let Some(future) = ArcFFI::as_ref(future_raw) else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: In this function we allocate a string, leak it (using into_raw
) and assign it to user-provided pointer.
The user is now responsible for freeing this string, but since this is a normal string there is no cpp-driver API for this (think cass_something_free
), so C-provided free
should be used.
This is not the first appearance of this pattern.
I have two concerns:
- Is it even legal to free Rust-allocated stuff using
free
from C? It very well might be, but I'm, not sure. - I think there won't be any issues caused by that pattern when developing custom allocator API from cpp-driver, but again I'm not sure. We'll need to be careful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, this is just testing API, I didn't notice. I'm sure this pattern appears in some public APIs, so the comment should still be valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: In this function we allocate a string, leak it (using
into_raw
) and assign it to user-provided pointer. The user is now responsible for freeing this string, but since this is a normal string there is no cpp-driver API for this (thinkcass_something_free
), so C-providedfree
should be used.
I introduced testing_free_host
which should be called on a pointer obtained from testing_future_get_host
. User does not need to call C free
. Or am I understanding your concerns wrong?
Note: In the next commit testing_free_host
and testing_free_contact_points
are replaced with testing_free_cstring
which can be used instead of them. These two functions did the same, so I introduced a common function (with a more suitable name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or am I understanding your concerns wrong?
In testing your testing_free_host
, but we have do places in the public API where the string is filled by us, and freed by the user with free
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I'll need to check that, because in the examples of such pattern I have on top of my head, the string is already allocated and owned by some other struct (which frees the string when dropped/free'd). I'll do it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@muzarski Any findings since?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I forgot about it. I've inspected all write_str_to_c
calls and did not find anything suspicious. We always provide a pointer to data that is owned by some structure. We never do something like:
- allocate the string
- pass the ownership to the user
- user does not have a method to free the string (leak)
afed812
to
665266c
Compare
Rebased on master and added |
// None only for tests - currently no way to mock coordinator in rust-driver. | ||
// Should be able to do so under "cpp_rust_unstable". | ||
pub(crate) coordinator: Option<Coordinator>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 If you believe it should be added to Rust driver, go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that convinced it should be added. WDYT about this Option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike that we allow for a special case that is possible only in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll try to do it. I don't want it to block this PR though.
#[unsafe(no_mangle)] | ||
pub unsafe extern "C" fn testing_future_get_host( | ||
future_raw: CassBorrowedSharedPtr<CassFuture, CConst>, | ||
host: *mut *mut c_char, | ||
host_length: *mut size_t, | ||
) { | ||
let Some(future) = ArcFFI::as_ref(future_raw) else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@muzarski Any findings since?
c2ab3c9
to
fd1eebd
Compare
To get the support for enforcing/getting coordinator. Note: It looks like the size of ExecutionError increased to 152 bytes. We get the following clippy lint: ``` the `Err`-variant returned from this function is very large --> src/query_result.rs:64:10 | 64 | ) -> Result<Self, CassErrorResult> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ::: src/execution_error.rs:15:5 | 15 | Execution(#[from] ExecutionError), | --------------------------------- the largest variant contains at least 152 bytes | = help: try reducing the size of `execution_error::CassErrorResult`, for example by boxing large elements or replacing it with `Box<execution_error::CassErrorResult>` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err = note: `-D clippy::result-large-err` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::result_large_err)]` ``` We can satisfy the linter by returning Arc<CassErrorResult> - the caller already wraps the returned error in Arc.
There is also `cass_statement_set_node` but it cooperates with `cass_future_coordinator` which will be implemented later.
I decided to store coordinator as Option. This is because we have to somehow mock it in unit tests - rust-driver does not expose any way to mock the coordinator.
There is one tricky part that we need to handle: the coordinator is held by CassResult, which is stored under Mutex in CassFuture. In result, the reference we obtain in `CassFuture::with_waited_result` closure has a lifetime of the mutex guard (temporary lifetime during the function call). We need to extend the lifetime of returned coordinator - further reasoning is explained in the comment in code.
The suite contains 4 tests: - SetHostWithValidHostString -> calls cass_statment_set_host with valid ip addresses and port. Expects CASS_OK to be returned - SetHostWithInvalidHostString -> calls cass_statement_set_host with invalid ip addressed ("inavlid", "", NULL). Expects LIB_BAD_PARAMS to be returned - SetHostWithValidHostInet -> calls cass_statement_set_host_inet with valid ip addresses Expects CASS_OK. - SetHostWithInvalidHostInet -> calls cass_statement_set_host_inet with invalid CassInet struct. Expects LIB_BAD_PARAMS
This suite contains 5 tests: - SetHost -> it enforces "127.0.0.1:9042" host using cass_statement_set_host. Then, it fetches the rpc_address from system.local and checks whether it matches enforced address (twice). - SetHostInet -> the same as above, but using cass_statement_set_host_inet (CassInet instead of String). - SetNode -> executes "SELECT rpc_address from system.local" on random node. Then it gets the coordinator of this request (using cass_future_coordinator) and enforces this coordinator (cass_statement_set_node) on the same statement. Checks whether addresses match. - SetHostWithInvalidPort -> tries to enforce host with unknown port (8888). Expects LIB_NO_HOST_AVAILABLE. - SetHostWhereHostIsDown -> stops the node, and then tried to enforce it as a coordinator for some request. Expects LIB_NO_HOST_AVAILABLE.
It contains two tests. Both of the tests use 3-nodes cluster (single dc) and RF=3. Both of them try to enforce the "127.0.0.1" host for some read/write request with cl=LOCAL_QUORUM. - ErrorReadWriteTimeout -> It **pauses** two remaining nodes and expects server-side READ/WRITE_TIMEOUT - ErrorUnavailable -> It **stops** two remaining nodes and expects UNAVAILABLE server-side error.
Added rust utilities to obtain a stringified ip address of request coordinator from future. Implemented `get_host_from_future` on top of them. This utility method is used in integration tests. We cannot enable any yet - they require other features (e.g. filtering config methods).
Replaced `testing_free_contact_points` and `testing_free_host` with one common method `testing_free_cstring`.
In the next commit we will want to call Option<CassResultValue>::expect(). It requires Debug implementation.
The result is going to be initialized only once, thus we do not need to store it behind a mutex. We can use OnceLock instead. Thanks to that, we can remove the unsafe logic which extends the lifetime of `coordinator` reference in cass_future_coordinator. We now guarantee that the result will be immutable once future is resolved - the guarantee is provided on the type-level.
fd1eebd
to
d760c24
Compare
v1.1: Addressed @wprzytula comments |
Fixes: #249, Fixes: #242
Ref: #132
Implemented methods:
cass_statement_set_host
cass_statement_set_host_n
cass_statement_set_host_inet
cass_statement_set_node
cass_future_coordinator
Problems with implementing
cass_future_coordinator
cass_future_coordinator
returnsCassNode*
which should be borrowed fromCassFuture
and live as long as underlyingCassResult
lives.The problem is that
CassResult
is stored under aMutex
. This is why the obtained reference ofCoordinator
has very short lifetime - it's the lifetime of acquiredMutexGuard
. But we need to return something with longer lifetime - our safe FFI API (with borrow-checker) starts to complain.In the commit where I implement
cass_future_coordinator
Iunsafe
ly extend the lifetime of the coordinator. We can do that, because we are guaranteed that returned pointer will be valid as long asCassFutureResult
is valid (see SAFETY comment in the code).However, there is a way to omit this unsafe code and represent the
CassFutureResult
immutability guarantees (after the future is resolved) on a type level. Last commit changesCassFuture
a bit, so theCassFutureResult
is stored insideOnceLock
, and not insideMutex
. Only then, can we remove the unsafe code responsible for extending the lifetime incass_future_coordinator
. This commit serves as a suggestion of one of (probably many) solutions to this problem. If we decide to do that, I'll probably split this commit into multiple commits before merging.Integration tests
Enabled 3 test suites:
StatementTests
,StatementNoClusterTests
andServerSideFailureThreeNodeClusterTests
- 11 tests in total! The logic of each test is explained in commit messages.Pre-review checklist
.github/workflows/build.yml
ingtest_filter
..github/workflows/cassandra.yml
ingtest_filter
.