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

[API Proposal]: WebSocketException to expose the underlying HTTPResponse or the response status #62474

Closed
vicancy opened this issue Dec 7, 2021 · 4 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net
Milestone

Comments

@vicancy
Copy link

vicancy commented Dec 7, 2021

Background and motivation

When customers connect to the Azure Web PubSub service using ClientWebSocket, there is a need to check if the connection failure is reconnectable or not. For example, if the HTTP upgrade request returns 400, this WebSocket connection is not reconnectable, while when the HTTP upgrade request returns 5xx, this might be caused by intermittent network issues and it is reconnectable. However, in the current design

throw new WebSocketException(WebSocketError.NotAWebSocket, SR.Format(SR.net_WebSockets_Connect101Expected, (int)response.StatusCode));
, the WebSocketException simply throws WebSocketError.NotAWebSocket which makes it hard for customers to differentiate from different scenarios. Customers have to parse the net_WebSockets_Connect101Expected string to get out the status code...

API Proposal

namespace System.Net.WebSockets
{
    public sealed class WebSocketException : Win32Exception
    {
        public HttpResponseMessage UpgradeResponse { get; }
    }
}

API Usage

try {
    // Start the WebSocket connection
} catch (WebSocketException e) {
   if (e.UpgradeResponse.StatusCode >= 500) {
      // Try reconnecting...
   }
}

Alternative Designs

No response

Risks

No response

@vicancy vicancy added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 7, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net untriaged New issue has not been triaged by the area owner labels Dec 7, 2021
@ghost
Copy link

ghost commented Dec 7, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

When customers connect to the Azure Web PubSub service using ClientWebSocket, there is a need to check if the connection failure is reconnectable or not. For example, if the HTTP upgrade request returns 400, this WebSocket connection is not reconnectable, while when the HTTP upgrade request returns 5xx, this might be caused by intermittent network issues and it is reconnectable. However, in the current design

throw new WebSocketException(WebSocketError.NotAWebSocket, SR.Format(SR.net_WebSockets_Connect101Expected, (int)response.StatusCode));
, the WebSocketException simply throws WebSocketError.NotAWebSocket which makes it hard for customers to differentiate from different scenarios. Customers have to parse the net_WebSockets_Connect101Expected string to get out the status code...

API Proposal

namespace System.Net.WebSockets
{
    public sealed class WebSocketException : Win32Exception
    {
        public HttpResponseMessage UpgradeResponse { get; }
    }
}

API Usage

try {
    // Start the WebSocket connection
} catch (WebSocketException e) {
   if (e.UpgradeResponse.StatusCode >= 500) {
      // Try reconnecting...
   }
}

Alternative Designs

No response

Risks

No response

Author: vicancy
Assignees: -
Labels:

api-suggestion, area-System.Net, untriaged

Milestone: -

@svick
Copy link
Contributor

svick commented Dec 7, 2021

I think the discussion about adding StatusCode to HttpRequestException (which was done in .Net 5) is relevant here: #911.

In particular, one comment (#911 (comment)), which explains why it should be just the HttpStatusCode, and not the whole HttpResponseMessage:

Having an HttpResponseMessage in an exception means that it will keep connections open etc. So, then, do we add an Dispose() method to the HttpRequestException? It isn't IDisposable at all. And having a pattern where complex types are inside exceptions is very confusing.

So, I would suggest just adding the 'StatusCode'.

@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Dec 7, 2021
@karelz karelz added this to the 7.0.0 milestone Dec 7, 2021
@karelz
Copy link
Member

karelz commented Dec 7, 2021

Triage: This will help make WebSockets apps better. Should be simple. We need to first finalize the API.

@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.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 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

4 participants