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

Expose ClientWebSocket Upgrade Response Headers #28331

Closed
Yucked opened this issue Jan 7, 2019 · 17 comments
Closed

Expose ClientWebSocket Upgrade Response Headers #28331

Yucked opened this issue Jan 7, 2019 · 17 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net
Milestone

Comments

@Yucked
Copy link

Yucked commented Jan 7, 2019

Summary:

The API proposal comes after I was required to read response headers after making a WS handshake. Currently there is no way to see what response headers were returned after connecting to a WS server.

Rational & Usage:

For my use case, the current usage would be to figure out if a session was resumed by reading the response headers after re-establishing a WS connection.

Here is where the use case is properly described: https://github.com/Frederikam/Lavalink/blob/master/IMPLEMENTATION.md#resuming-lavalink-sessions

Further more, it can allow people to read any custom headers sent in response.

Proposed API:

Expose a property called Headers in ClientWebSocket/WebSocket class.

        public HttpResponseHeaders Headers
        {
            get
            {
                if (_headers == null)
                    throw new NullReferenceException("WebSocket connection hasn't started yet.");

                return _headers;
            }
        }

        private HttpResponseHeaders _headers;

And where headers are validated: https://github.com/dotnet/corefx/blob/master/src/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs#L181-L183

The _headers can be set once the required headers are validated.

                ValidateHeader(response.Headers, HttpKnownHeaderNames.Connection, "Upgrade");
                ValidateHeader(response.Headers, HttpKnownHeaderNames.Upgrade, "websocket");
                ValidateHeader(response.Headers, HttpKnownHeaderNames.SecWebSocketAccept, secKeyAndSecWebSocketAccept.Value);

                _headers = response.Headers;

Original Post:

Hi! Pardon me if I opened the issue in the wrong repo.

I was wondering if there was a way to get response headers after ws handshake was completed.
Something like WebSocketReceiveResult.Headers or similar.

Not sure if I'm looking in the right place. Any help will be appreciated.

Cheers!

@rmkerr
Copy link
Contributor

rmkerr commented Jan 7, 2019

Hey @Yucked! Do you mind clarifying what you mean by "Response headers after ws handshake was completed"? The headers on the 101 switching protocols response, or something else?

@Yucked
Copy link
Author

Yucked commented Jan 8, 2019

Hey @rmkerr

I was following this guide to implement resuming: https://github.com/Frederikam/Lavalink/blob/feature/resume/IMPLEMENTATION.md#resuming-lavalink-sessions

And in it, it says:

You can tell if your session was resumed by looking at the handshake response header Session-Resumed which is either true or false:
Session-Resumed: true

So, I thought perhaps they meant I should check my headers after doing (await ClientWebSocket.ConnectAsync()).Headers? but there is no such thing.

Perhaps something similar to this: websocket-client/websocket-client#161 ?

I'm sorry if I'm being extremely vague 😭

@rmkerr
Copy link
Contributor

rmkerr commented Jan 8, 2019

No problem! That info does help. Essentially what that is asking for is the last set of HTTP headers sent by the server before switching over to the WebSocket protocol. Unfortunately it doesn't look like we currently expose that information.

We do verify that a few headers required by the WebSocket protocol are included, but we don't look at any others:
https://github.com/dotnet/corefx/blob/adb4d48fe65d48b6dd1c2ff15c8a60e5f96bf266/src/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs#L175-L183

Adding the ability to review those headers does seem reasonable, but it would probably require an API change. Usually we try to make sure there's interest in that kind of change before adding the feature, so I'll leave this issue up for that purpose.

@rmkerr rmkerr changed the title ClientWebSocket Response Headers Expose ClientWebSocket Upgrade Response Headers Jan 8, 2019
@Yucked
Copy link
Author

Yucked commented Jan 9, 2019

Omg, thank you!

@Yucked
Copy link
Author

Yucked commented Feb 15, 2019

Hey. Just wondering how much interest is needed to get this in core 3.0?

@rmkerr
Copy link
Contributor

rmkerr commented Feb 15, 2019

I'm not sure. @karelz, this ask would require us to expose a new property. How much interest would we need to make that kind of change?

We do take contributions though -- if you are interested, you can try to get the change through API review yourself. That process is detailed here.

@Yucked
Copy link
Author

Yucked commented Feb 16, 2019

I read through the guide but it asks to open an issue first then wait for it to get approved. Should I make another issue on the same topic, wait for it to get approved then make a PR or just use this issue and open a PR?

@karelz
Copy link
Member

karelz commented Feb 16, 2019

@Yucked you can put your API proposal to the top post here.
Be warned: We're a bit under water, so no guarantee we will be able to help it push through into 3.0 ... but if hte API design is good and if it makes sense, we are definitely interested long-term (post-3.0), and maybe it will make it even earlier ... who knows ...

@Yucked
Copy link
Author

Yucked commented Feb 17, 2019

I've updated my post but unsure if the suggestion covers all areas that may be affected by the change. The only changes I can think of are:

  • Returning the response headers in connect async methods
  • Reflect the change on ClientWebSocket's ConnectAsync method

@karelz
Copy link
Member

karelz commented Oct 1, 2019

Triage: We need proper API proposal - see API process for examples.

@Yucked
Copy link
Author

Yucked commented Oct 3, 2019

@karelz Should I open a new issue for API Proposal?

@karelz
Copy link
Member

karelz commented Oct 3, 2019

@Yucked we can do it here - ideally, please edit top post (you can keep original text in separate section for history)

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@mleenhardt
Copy link

Just wanted to check on the status of this and what would be needed to make it move forward. We have a similar use case where we're sending a bearer token, setting it in the handshake response header and we need to be able to read it on the client.

@osakishore
Copy link

osakishore commented Aug 17, 2021

Any update on this, is there any possibility to read a response headers & response status code in the client side.

@karelz
Copy link
Member

karelz commented Aug 17, 2021

No update, it didn't make the cut for 6.0.

@peterthesaint
Copy link

peterthesaint commented Mar 1, 2022

Just wanted to check on the status of this and what would be needed to make it move forward. We also require a client (ClientWebSocket) to have the ability to access the response headers returned by the server in the handshake response on connection. In our project, the server uses this mechanism to inform the client of the sub-protocol version that will be used during subsequent communication.

@MihaZupan
Copy link
Member

This has been addressed by the new APIs added in #25918 (comment) (exposed status code and headers).
Please let us know if you're still missing any info.

@karelz karelz modified the milestones: Future, 7.0.0 Jul 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net
Projects
None yet
Development

No branches or pull requests

8 participants