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

fix(ext/flash): Fix request object returning uri property correctly after websocket upgrade #18519

Closed
wants to merge 5 commits into from

Conversation

andrewnester
Copy link
Contributor

Currently this is what's happening using the example from #18490

  1. When websocket connection established, the following function executes and adds the request with token 0 in context https://github.com/denoland/deno/blame/main/ext/flash/lib.rs#L1377
  2. After that websocket is upgraded (https://github.com/denoland/deno/blob/main/ext/flash/01_http.js#L425) which removes the request with token 0 from the context https://github.com/denoland/deno/blame/main/ext/flash/lib.rs#L1456
  3. open event is emitted after websocket upgraded is executed https://github.com/denoland/deno/blob/main/ext/flash/01_http.js#L435, therefore on open callbacks are executed later
  4. If we request request.uri option inside open callback, it is lazy loaded via this function https://github.com/denoland/deno/blob/main/ext/flash/01_http.js#L574 which requires request with token 0 in the server context which was removed on step 2. Thus panic.

This PR fixes this issue by preloading path before executing flash request.

Closes #18490

@bartlomieju
Copy link
Member

Thanks for the PR @andrewnester, we're currently in the process for migrating away from using the "flash" server (with #18511 being the prerequisite), so I'm not sure if we'll land this PR.

@bartlomieju
Copy link
Member

@andrewnester thank you so much for the PR, but this issue got fixed in #18568. I'm going to close it without a merge. I really appreciate the contribution, sorry I didn't let you know that we're working on it in the issue.

@bartlomieju bartlomieju closed this Apr 3, 2023
@andrewnester andrewnester deleted the issue-18490 branch April 4, 2023 11:31
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 this pull request may close these issues.

Panicked when trying to Deno.serve() a WebSocketServer
2 participants