Only serialize key when C function asks for a key #273
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For some operations, the DDS API requires a key instead of a sample ("dispose" is one of
those), and does this by invoking the "ddsi_serdata_from_sample" operation with kind set
to "SDK_KEY".
The Python binding is a bit unusual in that it serializes the data before calling into C
because that allows using Python code to serialize the data without having to call back
into Python code from C or being forced to interpret Python objects from C. A mismatch is
possible, and this causes failures if the serialized form of the key is not a prefix of
the serialized form of the full sample. For example:
@DataClass
class S(idl.IdlStruct, typename="S"):
v: int
k: int
key("k")
ih0 = dw.register_instance(S(0, 1))
Will misinterpret the bytes and treat the key value as 0 instead of as 1.
It can be dealt with in two ways:
* One is to continue always serializing a full sample in Python, fixing it up in the
serdata implementation if a serialized key is requested. This is small change in the
Python "clayer";
* The other is to serialize a key when a key is expected. This is somewhat fragile, but
the list of operations is small and the API rather stable.
This commit implements the second option, and mostly because it means the serializer will
not touch the non-key fields.
Signed-off-by: Erik Boasson <[email protected]>
dpotman
reviewed
Nov 28, 2024
| raise Exception("Cannot encode this type with version 0, contains xcdrv2-type structures") | ||
| use_version_2 = True | ||
|
|
||
| if self.keyless: |
There was a problem hiding this comment.
Is this early-out not needed in the generic seralized, can that function be called in SerializeKind.Key... mode for a keyless type?
| @@ -371,7 +371,7 @@ def wait_for_historical_data(self, timeout: int) -> bool: | |||
| raise DDSException(ret, f"Occured while waiting for historical data in {repr(self)}") | |||
|
|
|||
| def lookup_instance(self, sample: _T) -> Optional[int]: | |||
| ret = ddspy_lookup_instance(self._ref, sample.serialize()) | |||
| ret = ddspy_lookup_instance(self._ref, sample.serialize_key()) | |||
There was a problem hiding this comment.
I'd expect that this serialize also needs padding \0s, similar to pub.lookup_instance?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For some operations, the DDS API requires a key instead of a sample ("dispose" is one of those), and does this by invoking the "ddsi_serdata_from_sample" operation with kind set to "SDK_KEY".
The Python binding is a bit unusual in that it serializes the data before calling into C because that allows using Python code to serialize the data without having to call back into Python code from C or being forced to interpret Python objects from C. A mismatch is possible, and this causes failures if the serialized form of the key is not a prefix of the serialized form of the full sample. For example:
Will misinterpret the bytes and treat the key value as 0 instead of as 1.
It can be dealt with in two ways:
This commit implements the second option, and mostly because it means the serializer will not touch the non-key fields.
Intended to fix #269