Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

doc: Update doc for http.IncomingMessage.socket #5502

Closed
wants to merge 1 commit into from
Closed

doc: Update doc for http.IncomingMessage.socket #5502

wants to merge 1 commit into from

Conversation

kevinoid
Copy link

For HTTPS servers, the socket property of the IncomingMessage is a
tls.CleartextStream rather than a net.Socket. Update the documentation
to reflect this and note that the underlying socket can be accessed
through the socket property of the tls.CleartextStream.

Also update the documentation for getting the client authentication
details. verifyPeer was removed in df46c8e 2.5 years ago and
request.connection is a now-undocumented alias for request.socket.
Update these to the current names.

Reviewer Note: The socket property of tls.CleartextStream is not currently documented elsewhere (that I am aware of). If this is not intended to be part of the public API, is there an alternate way to get commonly used socket information (e.g. local and remote address and port) for HTTPS requests? If so, I can update and re-send the pull request using that method.

Thanks for considering,
Kevin

@bnoordhuis
Copy link
Member

The socket property of tls.CleartextStream is not currently documented elsewhere (that I am aware of). If this is not intended to be part of the public API, is there an alternate way to get commonly used socket information (e.g. local and remote address and port) for HTTPS requests?

The .socket property is only there so that tls.CleartextStream#address() and friends can delegate to it. Which should answer your second question with the caveat that there currently are no getters for localAddress and localPort (which are trivial to add however.)

@kevinoid
Copy link
Author

Ok. So would you prefer that I remove the reference CleartextStream.socket and update the pull request, or wait on a patch which provides a mechanism for users to get that sort of information and then update the pull request?

I find getting the message source to be an important operation to support, so I'd vote for the second, but there's no sense waiting if it's unlikely to be fixed. Either way. Let me know.

@bnoordhuis
Copy link
Member

I'll see if I can add localAddress etc. tomorrow. Like I said, it's trivial - most of the work is in writing the test.

@kevinoid
Copy link
Author

That's great. Thanks!

bnoordhuis added a commit to bnoordhuis/node that referenced this pull request May 20, 2013
Add localAddress and localPort properties to tls.CleartextStream.
Like remoteAddress and localPort, delegate to the backing net.Socket
object.

Refs nodejs#5502.
@bnoordhuis
Copy link
Member

Landed localAddress and localPort in d820b64.

For HTTPS servers, the socket property of the IncomingMessage is a
tls.CleartextStream rather than a net.Socket.  Update the documentation
to reflect this.

Also update the documentation for getting the client authentication
details.  verifyPeer was removed in df46c8e 2.5 years ago and
request.connection is a now-undocumented alias for request.socket.
Update these to the current names.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@kevinoid
Copy link
Author

Thanks again! I've updated the pull request to remove any mention of socket on CleartextStream. I didn't add mention of localAddress/localPort, since you already updated the documentation for CleartextStream I don't think users will have a hard time finding it, but I could mention it if you think it would be useful.

@jasnell jasnell added the doc label Aug 6, 2015
@jasnell jasnell added the P-3 label Aug 13, 2015
@jasnell
Copy link
Member

jasnell commented Aug 29, 2015

@bnoordhuis ... do you think we need this at this point?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants