Skip to content

Conversation

@Patryk27
Copy link

Spotted over at
#2624 (comment).

I'm 50/50 on whether we actually need that call or not (not a JNA expert either), but we're already doing that for uniffiHandleSuccess and it cannot hurt, so.

Note that tests (./docker/cargo-docker.sh test) fail with:

test uniffi_foreign_language_testcase_test_py ... ok
/mounted_workdir/target/tmp/uniffi_unary_result_alias-81c252357d8743a6/unary_result_alias.swift:360:46: error: unknown attribute 'unchecked'
fileprivate final class UniffiHandleMap<T>: @unchecked Sendable {
                                             ^
/mounted_workdir/target/tmp/uniffi_unary_result_alias-81c252357d8743a6/unary_result_alias.swift:360:56: error: use of undeclared type 'Sendable'
fileprivate final class UniffiHandleMap<T>: @unchecked Sendable {
                                                       ^~~~~~~~
test uniffi_foreign_language_testcase_test_swift ... FAILED
test uniffi_foreign_language_testcase_test_kts ... ok

failures:

---- uniffi_foreign_language_testcase_test_swift stdout ----
Creating testing out_dir: /mounted_workdir/target/tmp/uniffi_unary_result_alias-81c252357d8743a6
Error: running `swiftc` to compile bindings failed (cd "/mounted_workdir/target/tmp/uniffi_unary_result_alias-81c252357d8743a6" && "swiftc" "-emit-module" "-module-name" "unary_result_alias" "-o" "libtestmod_unary_result_alias.so" "-emit-library" "-swift-version" "5" "-Xcc" "-fmodule-map-file=/mounted_workdir/target/tmp/uniffi_unary_result_alias-81c252357d8743a6/unary_result_aliasFFI.modulemap" "-I" "/mounted_workdir/target/tmp/uniffi_unary_result_alias-81c252357d8743a6" "-L" "/mounted_workdir/target/tmp/uniffi_unary_result_alias-81c252357d8743a6" "-luniffi_unary_result_alias" "/mounted_workdir/target/tmp/uniffi_unary_result_alias-81c252357d8743a6/unary_result_alias.swift")


failures:
    uniffi_foreign_language_testcase_test_swift

... but this doesn't seem related to the changes here 👀

Spotted over at
mozilla#2624 (comment).

I'm 50/50 on whether we actually need that call or not (not a JNA expert
either), but we're already doing that for `uniffiHandleSuccess` and it
cannot hurt, so.
@Patryk27 Patryk27 requested a review from a team as a code owner October 28, 2025 09:03
@Patryk27 Patryk27 requested review from mhammond and removed request for a team October 28, 2025 09:03
@badboy
Copy link
Member

badboy commented Oct 28, 2025

Note that tests (./docker/cargo-docker.sh test) fail with:

It seems we just never actually updated that script to use the newer Docker container.
CI is fine on this.
So yeah, unrelated to the changes.

@bendk
Copy link
Contributor

bendk commented Oct 28, 2025

The code looks right to me, thanks.

Do you have any way to test these changes and see how it affects the error rate? Ideally, I'd like to test them before we merged. But I'd be okay with merging and then testing as well. The thing I'd really like to avoid is merging this now, not knowing if it affects anything or not and forgetting about the work altogether. A big reason for that is that if this does fix a bug, then we should audit the code and find other instances of missing write() calls.

@Patryk27
Copy link
Author

Makes sense -- I'll ask around if we can do A/B testing, I'll let you know tomorrow!

@Patryk27
Copy link
Author

Status report: got sick and didn't have chance to ask yet - we'll see next week 😅 🤒

@Patryk27
Copy link
Author

Patryk27 commented Nov 11, 2025

Status: we unfortunately can't carry out A/B testings yet, but we've rewritten all problematic APIs to be sync and - judging by Sentry logs - those don't crash anymore at all.

Since the codegen'ed sync APIs don't call .write() either (seems so?), I think the chances of that "missing" .write() being the culprit in async APIs are quite low, so I think it's better to close this - looks like a red herring more than a solution.

@Patryk27 Patryk27 closed this Nov 11, 2025
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.

3 participants