Conversation
When Store is closed, the Sync client should be closed too
Include Sync libraries when building Sync package
…eakage when user forgets to call close()
…ypes' in Python code
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.gh.lumenfield.work/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37bee34205
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
objectbox/sync.py
Outdated
| c.obx_sync_listener_error(self.__c_sync_client_ptr, None, None) | ||
| c.obx_sync_listener_login(self.__c_sync_client_ptr, None, None) | ||
| c.obx_sync_listener_login_failure(self.__c_sync_client_ptr, None, None) | ||
| c.obx_sync_listener_connect(self.__c_sync_client_ptr, None, None) | ||
| c.obx_sync_listener_disconnect(self.__c_sync_client_ptr, None, None) |
There was a problem hiding this comment.
In close(), the C cleanup functions are called unconditionally with self.__c_sync_client_ptr. Because a store-close listener is registered and users can also call SyncClient.close() manually, it’s easy to hit a second close (e.g., client.close() then store.close()), which will pass a NULL handle into the C API. That contradicts the docstring (“Does nothing if this sync client has already been closed”) and can lead to C-level errors or crashes if the native API does not accept NULL. Add a guard (if self.__c_sync_client_ptr is None: return) before invoking the C functions to make close truly idempotent.
Useful? React with 👍 / 👎.
| if store in Sync.__sync_clients: | ||
| raise ValueError('Only one sync client can be active for a store') | ||
| client = SyncClient(store, server_urls, filter_variables) | ||
| client.set_multiple_credentials(credentials_list) | ||
| Sync.__sync_clients[store] = client |
There was a problem hiding this comment.
client_multi_creds_multi_urls() stores the new client in Sync.__sync_clients and rejects future creations for the same store, but nothing ever removes that entry on SyncClient.close() or store close. As a result, once a client is closed, calling this factory again with the same store will always raise ValueError even though no client is active. This prevents restarting sync for a store without restarting the process; consider removing the store from the registry when the client closes.
Useful? React with 👍 / 👎.
…es with the latest C library
Before starting the test-session, initiate a Sync server with Docker.
Add a tag that disables/skips Sync tests when the build is not a 'Sync' variant.
os.getuid() is not defined for Windows. Instead set user_id = 0 when running on Windows.
It is a required field when Sync server (version 5.0) parses the model JSON
The -s flag should provide a verbose output from test executions.
Instead of using Docker to run the Sync server, download its executable from GitLab artifacts and run it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1b5cc48b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import ctypes.util | ||
| import os | ||
| import platform | ||
| from objectbox.version import Version | ||
| from ctypes import c_char_p | ||
| from typing import * |
There was a problem hiding this comment.
This import block no longer brings IntEnum into scope, but the file still defines enums like DbErrorCode(IntEnum) later; that causes a NameError during module initialization and prevents objectbox.c (and therefore package import) from loading in environments where dependencies are present. Restoring from enum import IntEnum is required for the module to initialize.
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| def SyncEntity(cls): | ||
| entity: _Entity = obx_models_by_name["default"][-1] # get the last added entity |
There was a problem hiding this comment.
SyncEntity always mutates obx_models_by_name["default"][-1], so it can mark the wrong entity (or raise KeyError) when users define entities in a non-default model or use a different decorator order. In those cases, importing the model can fail or sync flags get applied to an unrelated class instead of the decorated one.
Useful? React with 👍 / 👎.
| raise ValueError('Only one sync client can be active for a store') | ||
| client = SyncClient(store, server_urls, filter_variables) | ||
| client.set_multiple_credentials(credentials_list) | ||
| Sync.__sync_clients[store] = client |
There was a problem hiding this comment.
This method stores every created client in Sync.__sync_clients and gates creation on that map, but there is no corresponding removal path on SyncClient.close() or store close. After creating and closing one client, a second call with the same store will still raise ValueError even though no active client exists.
Useful? React with 👍 / 👎.
1 participant
No description provided.