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

There should be a way to use already connected Socket in HttpClient connectionFactory. #55562

Open
LacticWhale opened this issue Apr 24, 2024 · 7 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-io P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team type-enhancement A request for a change that isn't a bug

Comments

@LacticWhale
Copy link

  • Dart SDK Version 3.3.4
  • Issue related to dart core library "dart:io"

My library (socks proxy client) worked before class modifiers existed by implementing ConnectionTask making it constructor public.
Now ConnectionTask is final class and I am unable to pass already connected socket to connectionFactory.
Some code that used to work:

class SocketConnectionTask implements ConnectionTask<Socket> {
  SocketConnectionTask(this.socket);
  @override
  final Future<Socket> socket;

  @override
  void cancel() => socket.ignore();
}
httpClient.connectionFactory =
      (uri, proxyHost, proxyPort) async {
        // Returns instance of SocksSocket which implements Socket
        final client = SocksTCPClient.connect(...);
        
        // Secure connection after establishing Socks connection
        if(uri.scheme == 'https')
          return SocketConnectionTask((await client).secure(uri.host, ...));

        return SocketConnectionTask(client);
      };

I have no idea how to fix it.

@LacticWhale LacticWhale changed the title There should be a way to use already Socket in HttpClient connectionFactory. There should be a way to use already connected Socket in HttpClient connectionFactory. Apr 24, 2024
@lrhn lrhn added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-io type-enhancement A request for a change that isn't a bug labels Apr 25, 2024
@haodong404
Copy link

I have the same problem, and is it possible to provide an approach for constructing a ConnectionTask instance?

@LacticWhale
Copy link
Author

I think laziest solution will be making ConnectionTask constructor public. Anyway it only used in Socket and considering we can extend/implement our own Socket I don't see why ConnectionTask shouldn't be available to us.

@a-siva a-siva added the triaged Issue has been triaged by sub team label May 1, 2024
@brianquinlan
Copy link
Contributor

Let me see if I can recover our reasoning for making ConnectionTask final. It was probably an oversight.

@brianquinlan
Copy link
Contributor

Here is the relevant comment: https://dart-review.googlesource.com/c/sdk/+/291343/4/sdk/lib/io/socket.dart (line 533):

Final, data class.
Nobody needs to create another version.

This really is a data class so we should probably just make the constructor public. Any objections @lrhn ?

@lrhn
Copy link
Member

lrhn commented May 7, 2024

Reasonable to make constructor public.

A little worried that <S> is not constrained in any way. This is basically a genetic CancelableOperation, which isn't linked to sockets in any way except the naming of the future variable.

If anyone can use it as a pair of Future<AnyType> and void Function(), the name no longer makes much sense.

Not sure what to do about that. It's there any reasonable supertype of sockets that the type variable can be bounded by? If not, should we add one?

@brianquinlan
Copy link
Contributor

Not sure what to do about that. It's there any reasonable supertype of sockets that the type variable can be bounded by?

I don't think so. RawSocket only implements Stream, which seems overly broad.

If not, should we add one?

That seems more like a @lrhn question ;-) Would this be a supertype that exists only to indicate that the subclass is Socket-like?

@brianquinlan brianquinlan added the P2 A bug or feature request we're likely to work on label May 8, 2024
@lrhn
Copy link
Member

lrhn commented May 8, 2024

As I remember it, we have several places where we generalize over different socket types, in one way or another, maybe it's only the underlying implementation classes that share something.
Maybe we should have a supertype, the problem is that it should be called Socket, which we already use for something different from a SecureSocket.

So no great ideas off the top of my head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-io P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants