Move addNetworkThreadCompletionHook to IClientApi. #340
Conversation
| for( auto it : externalClients ) { | ||
| MutexHolder holder(lock); |
There was a problem hiding this comment.
Should this go outside of the loop to avoid creating a mutex holder in each iteration? I suppose if we expect the mode number of external clients to be zero and the next most common number to be one, then the answer is "no", it should stay in the loop.
There was a problem hiding this comment.
Those in theory should be the most common cases, but I'm not opposed to moving it out, either.
There was a problem hiding this comment.
This is no longer relevant with my most recent change.
| @@ -355,6 +355,11 @@ ThreadFuture<Reference<ICluster>> DLApi::createCluster(const char *clusterFilePa | |||
| }); | |||
| } | |||
|
|
|||
| void DLApi::addNetworkThreadCompletionHook(void (*hook)(void*), void *hook_parameter) { | |||
| // All completion hooks are handled on the local client | |||
| throw unsupported_operation(); | |||
There was a problem hiding this comment.
I believe that this is the weirdness that caused us to put addNetworkThreadCompletionHook in MultiVersionClientApi but not IClientApi in the first place. I think there was also some concern that it wouldn't work with older clients. It looks like this doesn't have the second issue, so I guess that's fine. I'm not sure how I feel about the first, but maybe it's the least evil.
There was a problem hiding this comment.
Ideally, I think we'd just call straight through to the external library to handle completion hook, but as you say they may not support it.
However, it might be the case that we don't run the network on clients that don't support the specified API version, and this function shouldn't be called prior to whatever API version it was introduced in. So maybe we can actually get away with implementing it that way.
There was a problem hiding this comment.
The above doesn't work because this function can be called even if using older API versions. I instead decided to change the implementation so that DLApi was responsible for managing callbacks at the external level.
|
|
||
| auto hookPair = std::pair<void (*)(void*), void*>(hook, hook_parameter); | ||
|
|
||
| MutexHolder holder(lock); // We could use the network thread to protect this action, but then we can't guarantee upon return that the hook is set. |
There was a problem hiding this comment.
I'm not sure I totally understand this comment.
There was a problem hiding this comment.
Unless it's just saying that this won't necessarily happen on the network thread.
There was a problem hiding this comment.
Running it on the network thread would allow us to skip the lock (and it's how we serialize most things in ThreadSafe), but it seemed more sensible to me that the hook be definitely set when this function returned. Running it on the network thread would instead have it be done at some point in the future.
bindings/c/fdb_c.cpp
Outdated
| @@ -42,7 +42,8 @@ int g_api_version = 0; | |||
| #define CLUSTER(c) ((ICluster*)c) | |||
| #define TXN(t) ((ITransaction*)t) | |||
|
|
|||
| #define API (MultiVersionApi::api) | |||
| #define API ((IClientApi*)MultiVersionApi::api) | |||
|
|
|||
There was a problem hiding this comment.
Sorry, I guess I should have noted this earlier, but it might be helpful if there were a comment here explaining why we are casting down (up?) to an IClientApi here. Also, was the extra whitespace intentional?
No description provided.