Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Kotlin error handling in async functions #1614

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

mhammond
Copy link
Member

Seems like it probably fixes #1611 but I'm really not sure what I am doing :) cc @Hywan, does this look OK to you?

@mhammond mhammond requested a review from a team as a code owner June 20, 2023 14:38
@mhammond mhammond requested review from badboy and removed request for a team June 20, 2023 14:38
@@ -26,12 +26,12 @@ internal interface UniFfiFutureCallback{{ callback_param|ffi_type_name }} : com.

internal class {{ result_type|future_callback_handler }}(val continuation: {{ result_type|future_continuation_type }})
: UniFfiFutureCallback{{ callback_param|ffi_type_name }} {
override fun invoke(_callbackData: USize, returnValue: {{ callback_param|ffi_type_name_by_value }}, callStatus: RustCallStatus.ByValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. The definition of callback_param looks wrong.

In AsyncTypes.py, we have this for the argument:

        {%- match result_type.return_type -%}
        {%- when Some(return_type) -%}
        {{ return_type.ffi_type().borrow()|ffi_type_name }}
        {%- when None -%}
        ctypes.c_uint8
        {%- endmatch -%}

No result type generates ctypes.c_uint8 which seems to accept a null value.

In AsyncType.swift:

 returnValue: {{ result_type.future_callback_param().borrow()|ffi_type_name }},

which fallbacks to Ffi::Uint8, which seems to accept a null value.

The rest is ensured by the when Some(return_type) dance.

Either you change how callback_param is defined, or I would say this patch is valid :-p. I'm really surprised that it works with Swift. Didn't dig that much though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems right to me. The return FFI type is a pointer, since the parameter is an Arc<>. Errors get returned through a out-param and when the Rust function returns an error it needs to return some placeholder value for the pointer, which is null. This is different than either a void return or an Option<Arc<>>.

checkCallStatus() will throw an exception in the error case, so returnValue will never actually be touched. I haven't looked at the Swift code, but maybe Swift allows the null pointer as long as it's not accessed.

@@ -26,12 +26,12 @@ internal interface UniFfiFutureCallback{{ callback_param|ffi_type_name }} : com.

internal class {{ result_type|future_callback_handler }}(val continuation: {{ result_type|future_continuation_type }})
: UniFfiFutureCallback{{ callback_param|ffi_type_name }} {
override fun invoke(_callbackData: USize, returnValue: {{ callback_param|ffi_type_name_by_value }}, callStatus: RustCallStatus.ByValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems right to me. The return FFI type is a pointer, since the parameter is an Arc<>. Errors get returned through a out-param and when the Rust function returns an error it needs to return some placeholder value for the pointer, which is null. This is different than either a void return or an Option<Arc<>>.

checkCallStatus() will throw an exception in the error case, so returnValue will never actually be touched. I haven't looked at the Swift code, but maybe Swift allows the null pointer as long as it's not accessed.

@mhammond mhammond merged commit 68107e3 into mozilla:main Jun 21, 2023
@mhammond mhammond deleted the kotlin-async-struct branch June 21, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot return struct using Result in async function
3 participants