Conversation
AYAHASSAN287
requested review from
Ivansete-status,
NagyZoltanPeter and
fbarbu15
Ivansete-status
left a comment
Ivansete-status
left a comment
There was a problem hiding this comment.
LGTM! Thanks for it! 🙌
|
|
||
| ffi = FFI() | ||
|
|
||
| ffi.cdef(""" |
There was a problem hiding this comment.
I wonder if we could load the header from a file, instead of having it hard-coded? That would make it a bit more dynamic.
I expect the header to be in vendor/logos-delivery/liblogosdelivery/liblogosdelivery.h
|
fyi @igor-sirotin |
| def _make_cb(py_callback): | ||
| def c_cb(ret, char_p, length, userData): | ||
| msg = ffi.buffer(char_p, length)[:] | ||
| py_callback(ret, msg) | ||
|
|
||
| return CallbackType(c_cb) |
There was a problem hiding this comment.
Idk how to make it, but it would be great if every API call could handle it's callback, excluding outer caller. It is because that such callbacks delivers information about success/failure and possibly return value of the API call.
Without it will be hard to write straightforward test cases.
@Ivansete-status WDYT? Can we make synch wait for callbacks inside the API interface functions?
There was a problem hiding this comment.
I could wait till callback is returned but I will need to know the expected return time for each operation to catch if the return is late
There was a problem hiding this comment.
Yes, it's a problem as we cannot say exact min/max times, but in logos-delivery-module I used 30sec timeout for each callback to happen.
|
The metrics warning should disappear in latest version |
NagyZoltanPeter
left a comment
NagyZoltanPeter
left a comment
There was a problem hiding this comment.
I found some stuff for you may consider before going further. Let me know if I can help with sthg.
| def create_and_start(cls, config: dict, create_cb, start_cb): | ||
| node = cls.create_node(config, create_cb) | ||
| rc = node.start_node(start_cb) | ||
| return node, rc |
There was a problem hiding this comment.
This seems a bit optimistic design.
Suppose that only after creat_cb called you can be sure that node is created and available.
While also this proc returns but still start_node can take up to 10 secs.
So anything is called after this must wait the outcome of start_cb.
There was a problem hiding this comment.
I can add this validatin in the wrapped and the wait
| def set_event_callback(self, py_callback): | ||
| def c_cb(ret, char_p, length, userData): | ||
| msg = ffi.buffer(char_p, length)[:] | ||
| py_callback(ret, msg) | ||
|
|
||
| cb = CallbackType(c_cb) | ||
|
|
||
| lib.logosdelivery_set_event_callback( | ||
| self.ctx, | ||
| cb, | ||
| ffi.NULL, | ||
| ) |
There was a problem hiding this comment.
I would recommend to use the logosdelivery_set_event_callback in conjunction with create_node (if succeded) and not as a separate callable.
Or do you want it to change the event handler over time during the tests?
Still something to consider, the logosdelivery_set_event_callback is not a thread safe operation (even if it is just two pointer assignments in nim side).
| def stop_and_destroy(self, callback): | ||
| stop_rc = self.stop_node(callback) | ||
| if stop_rc != 0: | ||
| raise RuntimeError(f"Stop failed (ret={stop_rc})") | ||
|
|
||
| destroy_rc = self.destroy(callback) | ||
| if destroy_rc != 0: | ||
| raise RuntimeError(f"Destroy failed (ret={destroy_rc})") |
There was a problem hiding this comment.
This sequence can crash, because stop can take a while on nim side (callback will notify), but destroy is kind of immediate regardless of state of stop.
@Ivansete-status we might need to consider to protect this on FFI side...
There was a problem hiding this comment.
For now I think we need to control that destroy is invoked when stop_node is completed. This can be made, for now, within this stop_and_destroy def.
Yes good point!, we can enhance FFI to makesure destroy wait until there is not in-flight ffi requests.
Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>
4 participants
Summary
This PR introduces a Python CFFI wrapper around liblogosdelivery.so, exposing core node lifecycle and subscription APIs through a NodeWrapper class.
Each API was tested a basic test to make sure it's working
Exposed APIs
Key Functions
create_and_start
Convenience classmethod that creates and starts a node in one step
stop_and_destroy
Instance method that cleanly stops and destroys the node, raising on failure
Observations
Manual Library Handling
liblogosdelivery.so is currently built manually and copied into the bindings repository.
Metrics Warning on create_node
The following message appears when calling logosdelivery_create_node:
Following improvement:
Additional logging can be introduced to improve debugging and traceability.
Null pointer checks may also be added to strengthen safety and error handling.