Add a converter from Dart `Stream` to `NSInputStream` #1555

brianquinlan · 2024-09-13T00:54:53Z

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

github-actions · 2024-09-13T00:56:23Z

PR Health

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage

⚠️

File	Coverage
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_protocol.dart	💔 Not covered
pkgs/objective_c/lib/objective_c.dart	💔 Not covered
pkgs/objective_c/lib/src/ns_input_stream.dart	💔 Not covered
pkgs/objective_c/lib/src/objective_c_bindings_generated.dart	💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers

⚠️

// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
pkgs/objective_c/lib/src/ns_input_stream.dart

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_enum_int_mimic_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/swift2objc/lib/src/config.dart
pkgs/swift2objc/lib/src/generate_wrapper.dart
pkgs/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swift2objc/lib/src/generator/generator.dart
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_initializer_declaration.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_property_declaration.dart
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart
pkgs/swift2objc/lib/src/transformer/_core/utils.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_property.dart
pkgs/swift2objc/test/unit/parse_initializer_param_test.dart

This check can be disabled by tagging the PR with skip-license-check.

Package publish validation ✔️

Package	Version	Status
package:ffi	2.1.3	already published at pub.dev
package:ffigen	15.0.0-wip	WIP (no publish necessary)
package:jni	0.12.0	already published at pub.dev
package:jnigen	0.12.0	already published at pub.dev
package:native_assets_cli	0.9.0-wip	WIP (no publish necessary)
package:objective_c	3.0.0-wip	WIP (no publish necessary)
package:swift2objc	0.0.1-wip	WIP (no publish necessary)
package:swiftgen	0.0.1-wip	WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

coveralls · 2024-09-13T01:07:20Z

coverage: 90.984% (+0.1%) from 90.865%
when pulling 5941854 on brianquinlan:stream
into 0bf27dc on dart-lang:main.

liamappelbe · 2024-09-13T01:06:03Z

pkgs/objective_c/src/input_stream_adapter.m

+  NSStreamStatus _status;
+  BOOL _done;
+  NSError *_error;
+  id<NSStreamDelegate> _delegate; // This is a weak reference.


Does this need a __weak annotation? I thought ARC would treat this as a strong ref by default.

Also, if we're dealing with weak ref stuff, the test probably needs a ref counting test to make sure it's all working as expected. There aren't any ref counting tests in package:objective_c yet, so you'll have to copy across the utils from ffigen (objectRetainCount and doGC). Take a look at pkgs/ffigen/test/native_objc_test/arc_test.dart to see how they work. Ignore the stuff that uses counter (that's a bit redundant since I added objectRetainCount). Just check that objectRetainCount > 0, drop your Dart references, do a GC, then check that objectRetainCount == 0.

Does this need a __weak annotation? I thought ARC would treat this as a strong ref by default.

By default, _delegate is self so I think that you need the reference to be weak to prevent the circular reference:

https://developer.apple.com/library/archive/documentation/General/Conceptual/DevPedia-CocoaCore/ObjectOwnership.html

Also, if we're dealing with weak ref stuff, the test probably needs a ref counting test to make sure it's all working as expected. There aren't any ref counting tests in package:objective_c yet, so you'll have to copy across the utils from ffigen (objectRetainCount and doGC). Take a look at pkgs/ffigen/test/native_objc_test/arc_test.dart to see how they work. Ignore the stuff that uses counter (that's a bit redundant since I added objectRetainCount). Just check that objectRetainCount > 0, drop your Dart references, do a GC, then check that objectRetainCount == 0.

By default, _delegate is self so I think that you need the reference to be weak to prevent the circular reference:

https://developer.apple.com/library/archive/documentation/General/Conceptual/DevPedia-CocoaCore/ObjectOwnership.html

Yeah, I mean shouldn't you add an explicit __weak annotation? The way it's written now, I think ARC will be treating this field as strong.

Got it. Fixed.

liamappelbe · 2024-09-13T01:06:52Z

pkgs/objective_c/pubspec.yaml

@@ -4,7 +4,7 @@

 name: objective_c
 description: 'A library to access Objective C from Flutter that acts as a support library for package:ffigen.'
-version: 2.0.0
+version: 3.0.0-wip


Why does this need to be a major version bump?

Doesn't the addition of any new classes affect the code that should be generated by package:ffigen? For example, if we release package:objective_c with this change, then existing versions of package:ffigen may use it and users may face duplicate symbols: NSStream defined in package:objective_c and NSStream defined by ffigened code.

Or do I misunderstand how this works?

Yeah, I guess we do need a major version bump. That's annoying. I've been treating this sort of thing as a minor version.

brianquinlan · 2024-09-13T17:00:53Z

Also, why does it say that the coverage of ns_input_stream.dart is 0%?

brianquinlan · 2024-09-13T22:42:58Z

I thought that objective-c objects were transmittable through send ports? Is the CI using an older flutter version, or something?

❌ test/ns_input_stream_test.dart: NSInputStream toNSInputStream empty read without open (failed)
  Invalid argument(s): Illegal argument in isolate message: object is unsendable - Library:'package:objective_c/src/internal.dart' Class: _ObjCObjectRef@165304826 (see restrictions listed at `SendPort.send()` documentation for more information)
   <- Instance of 'DartInputStreamAdapter' (from package:objective_c/src/objective_c_bindings_generated.dart)
   <- field stream in read.<anonymous closure> (from file:///Users/runner/work/native/native/pkgs/objective_c/test/ns_input_stream_test.dart)
   <- resultPort in Instance of '_RemoteRunner<(int, Uint8List, bool, NSStreamStatus, NSError?)>' (from dart:isolate)
  
  dart:isolate                          Isolate.run
  test/ns_input_stream_test.dart 22:18  read
  test/ns_input_stream_test.dart 77:21  main.<fn>.<fn>.<fn>.<fn>

brianquinlan · 2024-09-13T22:50:45Z

For test/interface_lists_test.dart, I was not intending to export DartInputStreamAdapter - it is an implementation detail of NSInputStreamStreamExtension.toNSInputStream.

Do you think that I should add a list of exceptions to the test?

liamappelbe · 2024-09-15T23:26:28Z

Also, why does it say that the coverage of ns_input_stream.dart is 0%?

Yeah, this is a known issue. Coveralls doesn't always pick up all the subprojects. I haven't made any progress in debugging it. It's very annoying.

liamappelbe · 2024-09-15T23:35:23Z

I thought that objective-c objects were transmittable through send ports? Is the CI using an older flutter version, or something?

Does the test pass locally? We have another test that sends ObjC objects through ports, and they pass on CI, so I don't think it's the flutter version. Sometimes capturing stuff in closures like this can cause problems, so you could try rewriting the test to make the Isolate.run lambda a top-level function, and explicitly send the stream over a port instead of implicitly capturing it in the lambda.

brianquinlan · 2024-09-23T17:47:21Z

@liamappelbe I'm guessing from your comments that you think that we should land this - I wasn't sure if this was general enough.

Does the test pass locally? We have another test that sends ObjC objects through ports, and they pass on CI, so I don't think it's the flutter version.

They do pass locally. I'll investigate further.

For test/interface_lists_test.dart, I was not intending to export DartInputStreamAdapter - it is an implementation detail of NSInputStreamStreamExtension.toNSInputStream.

Do you think that I should add a list of exceptions to the test?

Any thoughts about this?

liamappelbe · 2024-09-24T01:29:09Z

@liamappelbe I'm guessing from your comments that you think that we should land this - I wasn't sure if this was general enough.

Seems fine to me. What makes it not general enough?

Does the test pass locally? We have another test that sends ObjC objects through ports, and they pass on CI, so I don't think it's the flutter version.

They do pass locally. I'll investigate further.

For test/interface_lists_test.dart, I was not intending to export DartInputStreamAdapter - it is an implementation detail of NSInputStreamStreamExtension.toNSInputStream.
Do you think that I should add a list of exceptions to the test?

Any thoughts about this?

Yeah, that makes sense. You can add it as an exception.

liamappelbe · 2024-09-27T01:43:34Z

pkgs/objective_c/lib/src/ns_input_stream.dart

+  /// the [Stream].
+  ///
+  /// > [!IMPORTANT]
+  /// > [NSInputStream.read_maxLength_] must called called from a different


nit: must ~~called~~ be called

liamappelbe · 2024-09-27T01:45:03Z

pkgs/objective_c/macos/Classes/objective_c.m

@@ -4,4 +4,5 @@

 // Relative import to be able to reuse the ObjC sources.
 // See the comment in ../objective_c.podspec for more information.
+#include "../../src/input_stream_adapter.m"


You'll also need to add this to ios/Classes/objective_c.m

liamappelbe · 2024-09-27T02:06:57Z

pkgs/objective_c/test/ns_input_stream_test.dart

+      }));
+}
+
+void main() {


Are you still working on the ref counting test? I want to make sure the __weak annotation works as we expect. Just create a stream (one that has _delegate = self), pipe some data through it, get a raw pointer to the stream and verify objectRetainCount != 0, drop the reference to the stream, call doGC(), then verify that the raw pointer now has a retain count of 0.

If blocks are involved, you might need to await a duration 0 future then do a second GC. The objectRetainCount function depends on a native function defined in pkgs/ffigen/test/native_objc_test/util.h, so you'll also need to include that in the test dylib.

Are you still working on the ref counting test? I want to make sure the __weak annotation works as we expect. Just create a stream (one that has _delegate = self), pipe some data through it, get a raw pointer to the stream and verify objectRetainCount != 0, drop the reference to the stream, call doGC(), then verify that the raw pointer now has a retain count of 0.

If blocks are involved, you might need to await a duration 0 future then do a second GC. The objectRetainCount function depends on a native function defined in pkgs/ffigen/test/native_objc_test/util.h, so you'll also need to include that in the test dylib.

@liamappelbe I think that this PR is ready except for this. How do you think that we should reuse the code from pkgs/ffigen? Should we move that functionality into a separate test-only package? Duplicate them in this package?

I would just import them directly using relative paths (other tests already do this). If that won't work in this case, just duplicate them.

@liamappelbe The objectRetainCount implementation depends on native code that would need to be built. Could I ask you to do the infrastructure work and I can modify my test afterwards?

FWIW, when I run the tests, the number of DartInputStreamAdapter deallocations match the number of allocations.

liamappelbe

LGTM. Just had a question about the test.

liamappelbe · 2024-10-03T01:21:23Z

pkgs/objective_c/test/ns_input_stream_test.dart

+    });
+
+    test('assign to non-self', () async {
+      inputStream.delegate = [1, 2, 3].toNSData();


I don't really understand how this delegate stuff works. Is this effectively swapping out the backing data stream of inputStream for this NSData? If so, the NSData contains the same data that the inputStream was constructed with, so the test could pass even if the delegate logic was broken. Can you change the NSData's data?

Just for my own understanding, how can you use an NSData as a NSStreamDelegate? It doesn't seem to conform to that protocol.

Also, can you explain the control flow that you expect in this case? It's super hard to follow (mostly because I'm not very experienced with the delegate pattern). It seems that NSStreamDelegate only has one method, and your implementation of that method at input_stream_adapter.m:162 only ever logs throws an error, or does nothing. What code actually invokes the methods on the delegate?

I don't really understand how this delegate stuff works. Is this effectively swapping out the backing data stream of inputStream for this NSData? If so, the NSData contains the same data that the inputStream was constructed with, so the test could pass even if the delegate logic was broken. Can you change the NSData's data?

Just for my own understanding, how can you use an NSData as a NSStreamDelegate? It doesn't seem to conform to that protocol.

Also, can you explain the control flow that you expect in this case? It's super hard to follow (mostly because I'm not very experienced with the delegate pattern). It seems that NSStreamDelegate only has one method, and your implementation of that method at input_stream_adapter.m:162 only ever logs throws an error, or does nothing. What code actually invokes the methods on the delegate?

I changed the delegate method to just passthrough to the delegate (if it is not self) and confirmed the behavior in tests. I also added NSStreamDelegate to the list of protocols that exist in package:objective_c.

Do you understand the global refcount failure?

Add a converter from Dart Stream to NSInputStream

8922f87

github-actions bot added package:ffigen package:objective_c labels Sep 13, 2024

brianquinlan requested a review from liamappelbe September 13, 2024 00:55

liamappelbe reviewed Sep 13, 2024

View reviewed changes

brianquinlan added 3 commits September 13, 2024 15:27

Clarify the deadlock behavior of toNSInputStream.

af7f981

Update ns_input_stream_test.dart

903a7de

Update ns_input_stream_test.dart

c8215be

brianquinlan added 4 commits September 26, 2024 15:51

Update input_stream_adapter.m

55788d7

Merge remote-tracking branch 'upstream/main' into stream

6c6383d

Update CHANGELOG.md

893d1ff

Merge remote-tracking branch 'upstream/main' into stream

3ac018f

liamappelbe reviewed Sep 27, 2024

View reviewed changes

brianquinlan added 4 commits September 27, 2024 10:33

More work

041c969

Fix

5893796

Fixes

78f8bba

Update objective_c.yaml

d40f878

github-actions bot added the type-infra A repository infrastructure change or enhancement label Sep 27, 2024

brianquinlan added 3 commits September 27, 2024 14:47

Update ns_input_stream.dart

a0b5769

Update ns_input_stream.dart

c7c5c35

Update ns_input_stream.dart

a02e729

brianquinlan added 14 commits September 27, 2024 15:38

Update ns_input_stream.dart

a34f3c8

Update ns_input_stream.dart

f9abb11

Update ns_input_stream_test.dart

3147c62

Update ns_input_stream_test.dart

51ad42b

More tries

2f145e8

Update ns_input_stream.dart

b3965ff

Update ns_input_stream_test.dart

0f683a4

Update ns_input_stream_test.dart

7bc0624

Update ns_input_stream_test.dart

365f095

Update ns_input_stream_test.dart

67d6f18

Merge remote-tracking branch 'upstream/main' into stream

f725e2a

Update objective_c_bindings_generated.dart

508b840

Merge branch 'main' into stream

4cef393

Fix

f4a5b74

github-actions bot removed the type-infra A repository infrastructure change or enhancement label Sep 30, 2024

brianquinlan added 11 commits September 30, 2024 14:47

Update pubspec.yaml

fae63ae

Fixes

4a5cf95

Update ns_input_stream_test.dart

16b38ad

Update input_stream_adapter.h

5b35857

Regenerate

c9e17ad

Add comment

c67c4b8

Update input_stream_adapter.m

1c1a3c8

Update input_stream_adapter.m

c4e42a1

Update input_stream_adapter.m

442af62

Update interface_lists_test.dart

d0f65b6

Update ns_input_stream_test.dart

254577d

liamappelbe approved these changes Oct 3, 2024

View reviewed changes

brianquinlan added 3 commits October 3, 2024 18:37

Fixes

93d688a

Update CHANGELOG.md

8069f02

Merge remote-tracking branch 'upstream/main' into stream

5941854

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a converter from Dart `Stream` to `NSInputStream` #1555

Add a converter from Dart `Stream` to `NSInputStream` #1555

brianquinlan commented Sep 13, 2024

github-actions bot commented Sep 13, 2024 •

edited

Loading

coveralls commented Sep 13, 2024 •

edited

Loading

liamappelbe Sep 13, 2024

brianquinlan Sep 13, 2024

liamappelbe Sep 15, 2024

brianquinlan Sep 26, 2024

liamappelbe Sep 13, 2024

brianquinlan Sep 13, 2024

liamappelbe Sep 15, 2024

brianquinlan commented Sep 13, 2024

brianquinlan commented Sep 13, 2024

brianquinlan commented Sep 13, 2024

liamappelbe commented Sep 15, 2024

liamappelbe commented Sep 15, 2024

brianquinlan commented Sep 23, 2024

liamappelbe commented Sep 24, 2024

liamappelbe Sep 27, 2024

liamappelbe Sep 27, 2024

brianquinlan Oct 1, 2024

liamappelbe Sep 27, 2024

brianquinlan Oct 1, 2024

liamappelbe Oct 1, 2024

brianquinlan Oct 1, 2024

liamappelbe left a comment

liamappelbe Oct 3, 2024

brianquinlan Oct 4, 2024 •

edited

Loading

Add a converter from Dart Stream to NSInputStream #1555

Are you sure you want to change the base?

Add a converter from Dart Stream to NSInputStream #1555

Conversation

brianquinlan commented Sep 13, 2024

github-actions bot commented Sep 13, 2024 • edited Loading

PR Health

coveralls commented Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianquinlan commented Sep 13, 2024

brianquinlan commented Sep 13, 2024

brianquinlan commented Sep 13, 2024

liamappelbe commented Sep 15, 2024

liamappelbe commented Sep 15, 2024

brianquinlan commented Sep 23, 2024

liamappelbe commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamappelbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianquinlan Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Add a converter from Dart `Stream` to `NSInputStream` #1555

Add a converter from Dart `Stream` to `NSInputStream` #1555

github-actions bot commented Sep 13, 2024 •

edited

Loading

coveralls commented Sep 13, 2024 •

edited

Loading

brianquinlan Oct 4, 2024 •

edited

Loading