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 #4191 make websockets work in angular in parallel with http #6092

Merged
merged 73 commits into from
Aug 26, 2023

Conversation

robnagler
Copy link
Member

@robnagler robnagler commented Jul 10, 2023

TODO:

Depends on radiasoft/pykern#388

@robnagler robnagler marked this pull request as ready for review July 14, 2023 17:58
@robnagler
Copy link
Member Author

  • This does not including caching of frames. It was pretty fast as is so good enough for using on alpha.
  • Logging is different than tornado requests. It logs the start and end of the requests, because we have different information. The entries are sequence numbers and can be tracked.
  • Includes some refactoring of requesting downloads, but those are the same (GET) because they don't happen often.

@robnagler
Copy link
Member Author

@e-carlin this is ready for another review. I think it could be merged (only on in dev). The next big issues are caching frames and pub/sub for runStatus. These should be separate concerns. The former is necessary for a prod release but not the latter.

One question I have is how frequently should the UI try restarts on the websocket. I think polling is fine, since it's a lightweight request. There might need to be a user alert in the UI that connection has been interrupted from more than, say, 30 seconds (server restart).

Copy link
Member

@e-carlin e-carlin left a comment

Choose a reason for hiding this comment

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

Only one small comment on a comment.

All of my click around testing worked. Obviously there are counltess cases I didn't manually test so it would be good for other people to click around edge cases in their apps.

One general comment, I find myself frequently opening the developer tools network tab to view what requests are made and what the content of those requests are. With websockets and a binary protocol that now doesn't work. What about adding a pkdc to parse_message and websocket_response that allows one to dump the message (with associated uri, seq num, etc)?

sirepo/package_data/static/js/sirepo.js Show resolved Hide resolved
@robnagler
Copy link
Member Author

robnagler commented Aug 26, 2023

One general comment, I find myself frequently opening the developer tools network tab to view what requests are made and what the content of those requests are. With websockets and a binary protocol that now doesn't work. What about adding a pkdc to parse_message and websocket_response that allows one to dump the message (with associated uri, seq num, etc)?

#6250

@robnagler robnagler enabled auto-merge (squash) August 26, 2023 20:00
@robnagler robnagler merged commit afd270d into master Aug 26, 2023
2 checks passed
@robnagler robnagler deleted the 4191-api-websockets branch August 26, 2023 20:16
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.

authState redirects to root if user dir not found change srunit to use websockets when enabled
3 participants