Error Return for Instance Interfaces => Set

rticommunity / rticonnextdds-connector-go

RTI Connector for Connext DDS is a lightweight technology that enables DDS data to be accessed with Go.

Other

26 stars 12 forks source link

Error Return for Instance Interfaces => Set #16

Closed metarsit-nutonomy closed 3 years ago

metarsit-nutonomy commented 5 years ago

Proposal:

Return proper error of C library

// SetString is a function that set a string to a fieldname of the samples
func (instance *Instance) SetString(fieldName string, value string) error {

    fieldNameCStr, **err** := C.CString(fieldName)
        if **err** != nil {
                return err
        }
    defer C.free(unsafe.Pointer(fieldNameCStr))

    valueCStr, **err** := C.CString(value)
        if **err** != nil {
                return err
        }
    defer C.free(unsafe.Pointer(valueCStr))

    err  := C.RTIDDSConnector_setStringIntoSamples(unsafe.Pointer(instance.output.connector.native), instance.output.nameCStr, fieldNameCStr, valueCStr)
        if **err** != nil {
                return err
        }

    return nil
}

Current behavior:

Currently we do not return anything else other than nil

Desired behavior:

Return actual error if C library does not work

Use case:

Without returning proper errors, it will potentially be a problem with error handling as we will never know whether that instance.Set() fails.

metarsit-nutonomy commented 5 years ago

@kyoungho Do you think this will be possible? This will start to be a problem for us if the error returns is not well handled.

kyoungho commented 5 years ago

@metarsit I think error checking for the cgo functions (C.CString) is possible. According to this (https://golang.org/cmd/cgo/#hdr-Go_references_to_C), it looks like C.CString uses malloc and it would return null if it fails. I think we can check a return value is null or not.

Regarding Connector C functions for setting and getting values, they do not return errors. Our Connector team is currently working on this. I will keep you updated through this issue report.

metarsit-nutonomy commented 5 years ago

@kyoungho Thank you! Do you know the timeline on that? If it will take sometime, I might implement some work around.

kyoungho commented 5 years ago

@metarsit Sure! I believe the fix will be included in our next Connector release and our target date is around November. The RTI issue ID for this is CON-77. Meanwhile, I can fix the error handling for the cgo part. Then, I will fix the Connector part after the release.

metarsit-nutonomy commented 5 years ago

@kyoungho That's great! I am looking forward to it! If there is anything you need my help with please let me know! I can help to review if that's necessary.

kyoungho commented 5 years ago

@metarsit Sounds good! It would be great if you can review.

metarsit-nutonomy commented 5 years ago

@kyoungho Hit me up or assign me as reviewer whenever! :)

kyoungho commented 5 years ago

@metarsit Will do!

kyoungho commented 4 years ago

@metarsit-nutonomy Finally, I had a chance to add error checking. The function definitions in the C library were updated to support this, so I updated them in the header accordingly. Can you review my commit when you have time? Thanks!

metarsit commented 3 years ago

@kyoungho Can you refer me to the commit? I will review it.

metarsit commented 3 years ago

@metarsit Sure! I believe the fix will be included in our next Connector release and our target date is around November. The RTI issue ID for this is CON-77. Meanwhile, I can fix the error handling for the cgo part. Then, I will fix the Connector part after the release.

Do you know whether this is still on track?

kyoungho commented 3 years ago

Yes, it is in the develop branch. I should merge it once you review. Please see the following commit. https://github.com/rticommunity/rticonnextdds-connector-go/commit/3aefbe68a0dbfa0b5a295f7d04adcdf0b0480a9c

metarsit commented 3 years ago

Okay. A few things. I have been making changes to master, so develop is pretty out of date.

I think if we want develop to be the playground, we should make develop the default branch so we make our changes in there. And once it is releasable, you merge it to master and tag it.

I am suggesting we rebasing develop (which might cause conflict to many people) but its for the better.

metarsit commented 3 years ago

Please fix the conflict and I will review the commit again, it is quite different from master now.

kyoungho commented 3 years ago

I resolved the conflicts and created a PR to merge develop to master. I tagged the current master version with v0.3 and will tag the merged one with v0.4 after the merge is complete. Please review the PR. Thanks!

https://github.com/rticommunity/rticonnextdds-connector-go/pull/37