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

The differences in request.connection between HTTP and HTTPS requests are confusing... #4650

Closed
andyburke opened this issue Jan 23, 2013 · 7 comments

Comments

@andyburke
Copy link

After reading #1055, I understand that request.connection differs between HTTP (a socket) and HTTPS (a tls.CryptoStream), however, this is confusing and necessitates code that is not very clear to handle the differences between the two.

I'm not sure what the proper approach to the problem is, but I wanted to open this issue to allow for some discussion.

One approach that could make things more consistent would be for request.connection to always be a stream (either a tls.CryptoStream in the case of HTTPS or just a Stream in HTTP) that has a socket member.

It's possible I don't know enough about the issue, but this bug primarily has to do with the hour of lost time trying to track down why things were not working properly in the differing cases of HTTP/HTTPS requests. The difference in the connection objects is not intuitive as things currently stand.

@TooTallNate
Copy link

Can you be a little more specific about which differences between the two objects you are talking about?

@andyburke
Copy link
Author

I specifically had an issue trying to access bytesWritten.

But looking at it more, it just seems odd to me that the connection can be two different types of object. One is a Stream, one is a Socket.

It seems to me that most server code should be treating HTTP and HTTPS requests exactly the same. Some indication that the request is HTTPS is, of course, necessary. But most code shouldn't care if the request came over HTTP or HTTPS.

I tend to think having a consistent type for the connection (either a Socket or a Stream) would be preferable to the current implementation where they differ depending on the type of request. If my reading of the code is correct, there'd also be less need for proxying in the tls.CryptoStream.

Obviously this would be a big change and I'm not familiar enough with the decisions that led to the current situation to offer expert advice (or a pull request). But please consider this to be a layman's perspective. I was confused by the differences in the connection object between HTTP/HTTPS requests and the reasons they were different were not clear to me. As a layman, I assumed that the connection objects were substantially similar between the two. That was an incorrect assumption. Perhaps the current implementation is how it must be and I'm just not aware of all the factors involved. But I also wanted to have a discussion to make sure there wasn't some way to make it more intuitive for laymen like me, while still satisfying those who know more about the issue.

@TooTallNate
Copy link

But looking at it more, it just seems odd to me that the connection can be two different types of object. One is a Stream, one is a Socket.

Well a "socket" is actually a type of "duplex stream", so they're both technically Streams. I agree with your general standpoint that the two objects should behave as close to identically as is practical.

If the tls stream does not define the bytesWritten property, then perhaps we should add that as well. Patch accepted. Are you noticing any other differences between the two Streams?

andyburke added a commit to andyburke-forks/node that referenced this issue Jan 23, 2013
This adds a proxy for bytesWritten to the tls.CryptoStream.  This
change makes the connection object more similar between HTTP and
HTTPS requests in an effort to avoid confusion.

See issue nodejs#4650 for more background information.
@andyburke
Copy link
Author

I've submitted PR #4654.

I'm still not sure that proxying various missing pieces as people encounter problems is the right approach. Or at least I'm not clear on how this is a good solution in the long run.

isaacs pushed a commit that referenced this issue Jan 25, 2013
This adds a proxy for bytesWritten to the tls.CryptoStream.  This
change makes the connection object more similar between HTTP and
HTTPS requests in an effort to avoid confusion.

See issue #4650 for more background information.
@isaacs
Copy link

isaacs commented Jan 25, 2013

Fixed by 595b597.

@isaacs isaacs closed this as completed Jan 25, 2013
@andyburke
Copy link
Author

I'm still not sure I would consider this issue closed.

I submitted PR #4654 because it was suggested that it would be accepted, and it's better than the status quo.

But in the long run, it seems to me that the request.connection should be the same type of object, or a subclass of an object on both HTTP and HTTPS requests as opposed to the current proxying strategy.

@isaacs
Copy link

isaacs commented Jan 25, 2013

That's not realistic. Socket and SecureConnection should expose an identical interface. You identified a way in which they do not, which is a bug, which you fixed (thanks!)

If you find any other discrepancies in the documented API of these two things, then that's also a bug, and it'll get fixed (by you or someone else).

JavaScript isn't really a strictly typed language. Even if they're the same class of object, they're doing slightly different things, so they may still end up with different code paths and thus slightly different results. However, crypto.SecureConnection should be a strict superset of the functionality on net.Socket, so whatever you can do with http, you can also do with https.

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

No branches or pull requests

3 participants