-
Notifications
You must be signed in to change notification settings - Fork 7.3k
The differences in request.connection between HTTP and HTTPS requests are confusing... #4650
Comments
Can you be a little more specific about which differences between the two objects you are talking about? |
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. |
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 |
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.
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. |
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.
Fixed by 595b597. |
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. |
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. |
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.
The text was updated successfully, but these errors were encountered: