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

GH-2120 Switch to using an Server-Sent Event endpoint for Console #2179

Merged
merged 27 commits into from
Aug 18, 2024

Conversation

granny
Copy link
Contributor

@granny granny commented Jul 29, 2024

This PR adds an SSE endpoint to reposilite for relaying log messages to the CLI console. The /api/console/execute endpoint is used to send commands. Closes #2120

TODO/More Info Needed

The checklists below are a mixture of talking points that I need more input on as well as "TODOs" that need to be resolved. Please excuse the mess..

Meta

  • /api/console/log is the endpoint. I'm open to ideas if you'd like to call it something else. It's good enough for now.
  • There's not really a OpenAPI Spec for SSE endpoints so I put some basic info for now.
  • Currently all event messages are sent under the "log" event. Honestly it's kind of repetitive with the endpoint also being "log", so it'd make sense to send all messages under the default "message" event if the endpoint ends up staying as /log. answered GH-2120 Switch to using an Server-Sent Event endpoint for Console #2179 (comment)
  • Should there be special formatting of the /api/console/execute endpoint if it's done through CLI? Currently it just outputs as REMOTE EXECUTION /api/console/execute from 127.0.0.1 since right now there's no way to differentiate between a remote execution and what was sent through CLI. answered GH-2120 Switch to using an Server-Sent Event endpoint for Console #2179 (comment)

Backend

  • Failed to close writer (java.lang.InterruptedException) shows up when stopping the application. Unrelated to my PR.
  • SseClient connections aren't properly closed when stopping the application. All connections are closed in the ReposiliteDisposeEvent event under ConsolePlugin.
  • SseClient connections aren't properly closed in general. They only close when an attempt is made to push an event (which throws a logged exception) All connections are sent a "ping" comment every second to verify that they are still connected. Javalin will handle closing the connection for us if the ping comment does not get received.

Frontend

  • Adds an extra dependency, extended-eventsource, which reimplements EventSource using Fetch. This is required to pass an authentication header to the SSE endpoint, since you can't do so with the built-in EventSource API. Seems to work fine.
  • Closing the browser tab or refreshing the page doesn't properly close the SSE connection. The backend fails to notice that there is no longer a connection, and ends up throwing a small CME error the next time a console command is sent from the same credentials. Closing the browser tab or refreshing the page properly closes the SSE connection now.
  • Console connection error - Make sure that WebSockets are enabled needs to be changed/reworded It has been adjusted.
  • Stop using built-in EventSource for readystate constants (the ones from extended-eventsource return undefined and I don't know why) Good to leave as is.
  • Currently if the SSE connection fails/errors/disconnects, then it doesn't attempt to reconnect. Should it? How many times should it try to reconnect before giving up? It won't attempt to reconnect.

Other

I took the liberty of renaming the internal CliEndpoint class to ConsoleWebSocketHandler, to keep file names consistent. I can revert this if it was left like that on purpose.

@granny
Copy link
Contributor Author

granny commented Jul 31, 2024

Backend changes

  • All SSE connections are now closed when shutting down reposilite.
  • All SSE connections now get pinged every second (Javalin closes any connections where the ping fails)

Frontend changes

  • Close sse connection before the page is reloaded/refreshed. This is technically not needed, but it stops an annoying error from appearing in the browser console.

Etc

I believe I've fixed all the errors/bugs. This PR is now ready for review.

@granny granny marked this pull request as ready for review July 31, 2024 11:02
@dzikoysk dzikoysk changed the title Switch to using an Server-Sent Event endpoint for Console GH-2120 Switch to using an Server-Sent Event endpoint for Console Jul 31, 2024
Copy link
Owner

@dzikoysk dzikoysk left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! I've left some minor comments regarding the implementation 🫶

@dzikoysk
Copy link
Owner

Currently all event messages are sent under the "log" event. Honestly it's kind of repetitive with the endpoint also being "log", so it'd make sense to send all messages under the default "message" event if the endpoint ends up staying as /log.

I think that's fine - it costs us nothing and and this endpoint will be most likely used only by our internal frontend implementation.

Should there be special formatting of the /api/console/execute endpoint if it's done through CLI? Currently it just outputs as REMOTE EXECUTION /api/console/execute from 127.0.0.1 since right now there's no way to differentiate between a remote execution and what was sent through CLI.

That would be a great enhancement, I guess we could add some sort of ExecutionSource enum that would indicate if command was invoked from cli/api/web ui. It's up to you if you address that, we can even move this to separate PR to not introduce more changes in the scope of this one :)

@dzikoysk dzikoysk merged commit 647bb7f into dzikoysk:main Aug 18, 2024
3 checks passed
@dzikoysk
Copy link
Owner

Thanks, great work! 🫶

@granny granny deleted the feat/add-sse-endpoint branch August 18, 2024 18:07
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.

Replace WebSockets with Server-Sent Events for console
2 participants