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

Implement WebSocket Denial Response extension #486

Open
daveisfera opened this issue Oct 6, 2023 · 8 comments · May be fixed by #487
Open

Implement WebSocket Denial Response extension #486

daveisfera opened this issue Oct 6, 2023 · 8 comments · May be fixed by #487

Comments

@daveisfera
Copy link

The value for code is hard coded to 403 when close happens while connecting

daveisfera added a commit to daveisfera/daphne that referenced this issue Oct 6, 2023
@daveisfera daveisfera linked a pull request Oct 6, 2023 that will close this issue
@Kludex
Copy link

Kludex commented Dec 19, 2023

@daveisfera
Copy link
Author

Daphne doesn't implement this: https://asgi.readthedocs.io/en/latest/extensions.html#websocket-denial-response.

I'm not sure what this is supposed to mean. The description sounds like it works and I've tested the change in the linked PR and it works

@carltongibson
Copy link
Member

OK, thanks for looking at it @Kludex.

@daveisfera I'm not sure what the problem here really is, or why/how the PR resolves it. Also there's no tests.

I think to progress it would help to step back and explain what the issue is. Then we can think about the appropriate response.

(Sorry if that seems a pain, but I'm slow sometimes, and I like to understand an issue before just merging a proposal. Thanks.)

@Kludex
Copy link

Kludex commented Dec 19, 2023

The OP wants to send a different status code if the connection is rejected. It can only be done with the WebSocket Denial Response extension.

@carltongibson carltongibson changed the title code is ignored when close happens while connecting Implement WebSocket Denial Response extension Dec 19, 2023
@carltongibson
Copy link
Member

Ok, so I can have a read.

@daveisfera fancy doing a fuller job here?

@Kludex
Copy link

Kludex commented Dec 19, 2023

If reference is needed, both Uvicorn and Hypercorn implement the extension.

@daveisfera
Copy link
Author

Honestly, I've moved onto a different approach, so if you're not interested in resolving this issue, then you can just close this

@carltongibson
Copy link
Member

Ok, thanks for the input. I'll have a read about the extension anyhow.

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

Successfully merging a pull request may close this issue.

3 participants