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

QuicConnection.RemoteEndPoint should be IPEndPoint, not EndPoint #43000

Closed
geoffkizer opened this issue Oct 3, 2020 · 9 comments
Closed

QuicConnection.RemoteEndPoint should be IPEndPoint, not EndPoint #43000

geoffkizer opened this issue Oct 3, 2020 · 9 comments

Comments

@geoffkizer
Copy link
Contributor

I think this was changed to EndPoint because we want to support connecting using DnsEndPoint as well as IPEndPoint. That seems reasonable, but once the connection is established, the user should be able to retrieve the actual remote IP using RemoteEndPoint.

@geoffkizer geoffkizer added this to the 6.0.0 milestone Oct 3, 2020
@ghost
Copy link

ghost commented Oct 3, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Oct 3, 2020
@geoffkizer
Copy link
Contributor Author

@karelz @scalablecory We should probably add a label for area-System.Net.Quic. How do we do this?

@danmoseley
Copy link
Member

If you collectively decide you want one just create it (click labels button) and update area owners markdown. Karel can update the tagging bot too. However you should make some effort to go back and relabel as many existing issues as possible so that the labeling bot has some hope of labeling them correctly - just search by keyword and then bulk relabel the relevant issues. Later, we will retrain the labeling bot.

@CarnaViire
Copy link
Member

@geoffkizer @scalablecory Do we want to change API to return IPEndPoint?, or we just want to update the value inside the property after we've connected?

@geoffkizer
Copy link
Contributor Author

The overall model here is a little weird.

You pass the remote (and optionally local) endpoint to the constructor. This can be an IPEndPoint or a DnsEndPoint. But we don't actually use the remote endpoint until you call ConnectAsync.

I wonder if we should make this work more like Socket.Connect, where you pass the remote endpoint to the Connect method itself. And possibly we have multiple convenience overloads for Connect, like Socket does.

I'm also wondering if there is an implicit multi-connect behavior here as there is with Socket when using hostname (as opposed to IPAddress)? If so, are we implementing this or is msquic?

Also it's weird that LocalEndPoint is nullable but RemoteEndPoint is not. Seems like we should be consistent here. These are both nullable on Socket; presumably they return null when not connected/bound. I wonder if it's actually better to throw here when not connected, so users don't have to deal with nulls here.

@wfurt
Copy link
Member

wfurt commented Apr 20, 2021

It seems like we added hook to HttpClient that people (among other things) can run HTTP on transports other than IP. (like UDS) Would we ever have similar need for QUIC? (and yes, it is annoying to cast to IPEndPoint 99% cases)

The nullable discrepancy make more sense to me. You need RemoteEndPoint to connect to. Specifying local details is optional. (for server scenarios we seems to have the Listener instead)
But if both properties are accessible before connection is connected, we may throw. After the connection is connected, we should have both and they. should not be null.

@scalablecory
Copy link
Contributor

I think if we add #48685, then QuicConnection might be abstract -- in that case, we should keep this as EndPoint. Otherwise if QuicConnection is a concrete implementation, we can have this as IPEndPoint.

@scalablecory
Copy link
Contributor

Triage: we should also consider LocalEndPoint at the same time.

@ManickaP
Copy link
Member

Closing, covered by #68902

@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants