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

websockets 14.0 support #1769

Merged
merged 6 commits into from
Nov 13, 2024
Merged

websockets 14.0 support #1769

merged 6 commits into from
Nov 13, 2024

Conversation

jcheng5
Copy link
Collaborator

@jcheng5 jcheng5 commented Nov 12, 2024

See #1766

The websockets package released 14.0 which breaks us when using shiny run --reload.

  • As @gadenbuie noticed, the process_requests callback has changed.
  • Even with that fixed (as in this PR's first commit), it prints a scary looking error message: EOFError: stream ends after 0 bytes, before end of line and EOFError: connection closed while reading HTTP request line. I think this is happening because Shiny's VS Code extension connects via a raw socket just to see if a newly launched Shiny process is listening for requests yet, and if the connection is successful, it immediately closes the connection. The new websockets seems not to like this.

Once we get this working, I think we also need to decide whether to require 14.0, or to allow older versions and adapt to whatever version is being used.

Some types changed, and process_request is totally different. However,
this still leaves a spurious, scary-looking error when using the Run
App button from the current version of the Shiny VS Code extension.
@jcheng5
Copy link
Collaborator Author

jcheng5 commented Nov 13, 2024

If we're OK with requiring websockets>=13.0, then this is actually pretty straightforward, as the websockets.asyncio submodule hasn't really changed between 13 and 14. Going back to websockets<13.0 is harder, as then we need code for both legacy and asyncio implementations, and for type checking to pass in both cases is pretty annoying.

@jcheng5 jcheng5 marked this pull request as ready for review November 13, 2024 00:56
@jcheng5 jcheng5 requested a review from gadenbuie November 13, 2024 00:56
No need to pass through to the background thread, we can just
change the loglevel on the main thread.
@jcheng5
Copy link
Collaborator Author

jcheng5 commented Nov 13, 2024

Oh I forgot to mention how I solved the scary error message: by defaulting the websocket logger to CRITICAL. In theory this could squash legit error messages (that are specific to auto-reloading).

https://github.com/posit-dev/py-shiny/pull/1769/files#diff-be8712ed65f19637d0738c3495776b73a667e8f9e32a6ae4dea92328fe906259R173-R182

You can set SHINY_AUTORELOAD_LOG_LEVEL=DEBUG if you need to reveal those messages.

@gadenbuie
Copy link
Collaborator

Oh I forgot to mention how I solved the scary error message: by defaulting the websocket logger to CRITICAL. In theory this could squash legit error messages (that are specific to auto-reloading).

It's hard to set this environment variable when you're debugging in VS Code, but I spent a while looking at both sides and I think ultimately this is the right choice, at least for now. I doubt Shiny users need to see any of the websockets debug messages; in fact, I'd think these messages are more likely to be noise or intimidating than useful. And in the edge cases where we or power users need to see them we still can.

Copy link
Collaborator

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@karangattu A note for QA: we should do some quick smoke tests in GitHub Codespaces, Posit Workbench, etc. before release just to be sure.

CHANGELOG.md Outdated Show resolved Hide resolved
@karangattu
Copy link
Collaborator

LGTM, thank you!

@karangattu A note for QA: we should do some quick smoke tests in GitHub Codespaces, Posit Workbench, etc. before release just to be sure.

Yes, I can test it as soon as it is merged. Or. do you want me to checkout the branch and test it against that?

@gadenbuie gadenbuie merged commit b3725f7 into main Nov 13, 2024
41 checks passed
@gadenbuie gadenbuie deleted the websockets-14-0 branch November 13, 2024 18:38
@jcheng5
Copy link
Collaborator Author

jcheng5 commented Nov 13, 2024

@gadenbuie Yeah, I ended up setting the environment variable in my ~/.zshrc. I was thinking about just leaving it on but it really is so distracting to have two stack traces appear every time you launch an app 😞

Wonder if websockets would be open to a PR that silences this particular case...

@gadenbuie
Copy link
Collaborator

Wonder if websockets would be open to a PR that silences this particular case...

They might be open to it. It's very similar to another case where this kind of error appears that was fixed in 14.1 released last night. python-websockets/websockets#1513

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.

3 participants