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

Peer addresses #17

Open
mmastrac opened this issue Sep 28, 2023 · 12 comments
Open

Peer addresses #17

mmastrac opened this issue Sep 28, 2023 · 12 comments

Comments

@mmastrac
Copy link
Contributor

The socket should provide the local and peer addresses for connections. For example, you may wish to know on which port the outgoing connection was established for logging or accounting purposes. In addition, it may be useful to know which IP address a wildcard connection resolved to (though obviously this will be 127.0.0.1 in most cases).

@jasnell
Copy link
Collaborator

jasnell commented Sep 28, 2023

I've been considering adding a socket.info property for precisely this kind of metadata and other use cases.

The local address would need to be optional as managed environments like Workers do not actually have a useful local address given the fact that the connection is proxied.

For the remote address, this could also be a bit tricky due to proxying if the runtimes view of the remote address is a local/private address.

I think for both of these I would make these: implementations MAY provide information on the local/remote addresses for connections.

The other use case I have for socket.info are stats like socket.info.bytesSent and socket.info.bytesReceived, primarily for debugging purposes.

@mmastrac
Copy link
Contributor Author

I agree -- MAY is the right term for this.

@jasnell
Copy link
Collaborator

jasnell commented Sep 30, 2023

Added in #19

const socket = connect('https://example.com');
await info = socket.opened;
console.log(info.remoteAddress);
console.log(info.localAddress);

@teabroker
Copy link

The other use case I have for socket.info are stats like socket.info.bytesSent and socket.info.bytesReceived, primarily for debugging purposes.

This kind of information is better to put into method called stat. It should be synchronous, because statistics could have zero values and doesn't need a connection to be established to be available.

let stat = socket.stat()

console.log(stat.bytesSent)
console.log(stat.bytesReceived)

It also could contain information about delays, number of packets, connection attempts, network errors, ping timeout and other common network statistics. It could be useful for debugging, testing, and for devTools purposes.

@jasnell
Copy link
Collaborator

jasnell commented Nov 5, 2023

The SocketInfo dictionary has been added providing these. It is currently only provided when the opened promise is resolved but I would also like to expose a socket.info property that also provides this. That would serve the same purpose as socket.stat. It does not need to be a method, it can simply be an attribute.

@dom96
Copy link
Collaborator

dom96 commented Nov 6, 2023

I'm not sure that kind of info should be provided by this API, instead it should be provided by dev tools (which presumably have other APIs they use to get this kind of info).

@mmastrac
Copy link
Contributor Author

mmastrac commented Nov 6, 2023

@dom96 This is critically necessary for server-side hosted applications in many cases.

@dom96
Copy link
Collaborator

dom96 commented Nov 7, 2023

I see. Then it makes sense, but I would say "MAY" (as you've written above) is definitely the way to go for these. My worry is we discourage implementors of this API if it becomes too big, so making this stuff optional sounds like a good way to go.

@mmastrac
Copy link
Contributor Author

mmastrac commented Nov 7, 2023

I think that's reasonable, as browsers may not wish to expose this information ever while servers may almost always.

@teabroker
Copy link

@jasnell

The SocketInfo dictionary has been added providing these. It is currently only provided when the opened promise is resolved but I would also like to expose a socket.info property that also provides this. That would serve the same purpose as socket.stat. It does not need to be a method, it can simply be an attribute.

I see several drawbacks is seen right now. The connection opened promise resolves only when connection is fully established. From § 3.3.3:

The opened attribute is a promise that is resolved when the socket connection has been successfully established, or is rejected if the connection fails. For sockets which use secure-transport, the resolution of the opened promise indicates the completion of the secure handshake.

It means that connection became a total blackbox when it stales. And when connection fails due to timeout, developers wouldn't have information for analysis.

So I think it's critical to have this property available since the moment the Socket instance was created. Am I right that you and other authors shares this point of view?

Second issue of having such property is it's dynamic behavior. Should the SocketInfo object be equal to itself conn.info === conn.info? And if yes, then these assertions should be valid:

const {info} = conn.info
const {bytesSent} = info;
bytesSent; // 0
await write(conn, "Hello"); // Send 5 bytes.
assert(conn.info === info);
assert(info.bytesSent !== bytesSent);

Though if conn.info === info, then it should not be a plain object, it should be an instance of SocketInfo. If it's a plain object it could be confusing for developers to find that some properties of detached info got changed over time. Now it's not obvious whether SocketInfo is a class or just a plain object. According to other W3C provided standards it should have its own class.

If conn.info !== info then it's better to be a method. While it's easier to use destruction to receive SocketInfo, in my opinion it would be less error prone to return it from a method.

I still think it's not a good idea to mix up connection details with a statistics in one object, for me it's very different kind of information and it's better to join all the statistic under a distinct object. Maybe as a property of SocketInfo. Could you explain the logic you follow?

@teabroker
Copy link

@dom96

My worry is we discourage implementors of this API if it becomes too big, so making this stuff optional sounds like a good way to go.

But underdeveloped standard could lead to a situation when developers would have to struggle fighting with the platform. When you building new API it's hard to predict what could be necessary, but network APIs are very common and we just can look around and collect best practices. Or if we don't know what the best practices are, then it's better to call developers who work in this field. Now it seems that socket could be very hard to control and debug. Do you have referential specifications you use to build this standard?

I could imagine some minimalistic runtimes like IoT, which could have only minimal set of APIs implemented. But I'm not sure whether these are the target platforms for WinterWG. So if this API is designed for some more complex runtimes then I'd suggest to focus on stability and predictability for developers, because when the platforms limits you it what makes it very hard to build reliable software. So I'm not sure that such kind of concerns should be on the first place, while I'm agree about the overall standard simplicity.

@jasnell
Copy link
Collaborator

jasnell commented Nov 12, 2023

@teabroker ... What I have in mind is this:

  • socket.info would be the same SocketInfo as returned by await socket.opened. Prior to the resolution of socket.opened, some properties on socket.info may be unset. The fact that await socket.opened returns socket.info is largely just a shortcut.
  • If we were to provide stats such as bytesWritten, bytesSent, etc, I would imagine it would (a) be optional and (b) would be exposed via something like socket.info.stats
  • The current addition of the peer addresses also allows for those to be optional.

Though if conn.info === info, then it should not be a plain object, it should be an instance of SocketInfo. If it's a plain object it could be confusing for developers to find that some properties of detached info got changed over time. Now it's not obvious whether SocketInfo is a class or just a plain object. According to other W3C provided standards it should h

SocketInfo is defined as a webidl dictionary, which means it is realized as a plain javascript object but with defined properties with documented semantics, even tho it is not an actual class. Can you expand on why you believe this wouldn't be sufficient?

... we just can look around and collect best practices

The key challenge there is that it's difficult to analyze what exactly is really necessary. For instance, the Node.js tls api has a broad range (~49) of configuration options that we just simply have no idea whether folks are actually using or not. It's our intention to keep the options and properties available via this API as minimal as possible for the time being, expanding them only when there is a clear need and specific requests. We do not want to add anything just because someone might need it.

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

No branches or pull requests

4 participants