Throw a StateError if ResourceHandle.to* is called more than once.

[brianquinlan]

Change

Throw a StateError if ResourceHandle.toFile(), ResourceHandle.toSocket(), ResourceHandle.toRawSocket() or ResourceHandle.toRawDatagramSocket() is called more than once.

Rationale

Calling ResourceHandle.toFile(), ResourceHandle.toSocket(), ResourceHandle.toRawSocket() or ResourceHandle.toRawDatagramSocket() more than once produces two distinct Dart objects that are backed by the same file descriptor, which will become invalid when one object is closed or GCed.

For example:

void main(List<String> args) async {
  final executeInternalCommand = DynamicLibrary.process().lookupFunction<
      Void Function(Pointer<Char>, Pointer<Void>),
      void Function(
          Pointer<Char>, Pointer<Void>)>('Dart_ExecuteInternalCommand');
  final gcNow = "gc-now".toNativeUtf8();

  final file = File('/tmp/file.txt');

  await file.create();
  final openFile = await file.open();

  final handle = ResourceHandle.fromFile(openFile);
  handle.toFile().writeStringSync('test');
  executeInternalCommand(gcNow.cast(), nullptr);  // Will collect the file produced by `toFile` .
  handle.toFile().writeStringSync('test');  // Error because the file descriptor is no longer valid.
}

Impact

It is unlikely that this will break any existing correct code.

Mitigation

Users can save the result of the ResourceHandle.toFile(), etc. and use that instead of calling ResourceHandle.toFile() again.

[devoncarew]

How would people know if ResourceHandle.toFile() was safe to call or not?

[itsjustkevin]

@vsmenon @Hixie @grouma any comments on this breaking change request?

[grouma]

This should not impact ACX. To @devoncarew's point, can we make this a compilation error or a lint?

[vsmenon]

Overall LGTM.

Is it also worth considering caching the object via weak ref?

[Hixie]

I know nothing about ResourceHandle so have no opinion.

[brianquinlan]

@devoncarew We have scaring language in the toFile documentation:

Extracts opened file from resource handle.

This can also be used when receiving stdin and stdout handles.

If this resource handle is not a file or stdio handle, the behavior of the returned [RandomAccessFile](https://api.dart.dev/stable/2.18.0/dart-io/RandomAccessFile-class.html) is completely unspecified. Be very careful to avoid using a handle incorrectly.

Other than that I'm not sure how safe we can make this - the use case is receiving a file descriptor from another process and then converting it in a socket/file/pipe/whatever.

[brianquinlan]

@vsmenon Do you think that people will be surprised if we cache the file? The file is mutable so calling it twice will result in sharing state. We'd also still have the issue that issues cannot do:

rh.toFile();
rh.toRawSocket(); // File descriptor already in use by the file!

[devoncarew]

the use case is receiving a file descriptor from another process and then converting it in a socket/file/pipe/whatever

It sounds like it's not a high volume use case, and that the expectation is that people will create the object, and convert it to another type, likely within one method? Otherwise it seems like it would be tough - if you were given a ResourceHandle from somewhere else - for you to know whether it was safe to call toFile() (or toSocket(), ...), or whether that call would instead throw.

Perhaps that issue could could be mitigated by API dartdocs? If we doc'd this behavior, then people using the API would be aware that those methods aren't idempotent and could plan for it.

[brianquinlan]

The actual PR has includes documentation changes:

  /// Since the [ResourceHandle] represents a single OS resource,
  /// none of [toFile], [toSocket], [toRawSocket], or [toRawDatagramSocket]
  /// must be called after a call to this method.

I should probably change s/must/can/ . PR is here:
https://dart-review.googlesource.com/c/sdk/+/257000/5/sdk/lib/io/socket.dart

[brianquinlan]

Fixed in fa53fb6