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

Split WebSocket and HTTP server - Breaking Change #230

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andig
Copy link
Contributor

@andig andig commented Oct 7, 2023

Fix #176

I've attempted to multiplex OCPP and HTTP traffic on a single port. This is currently not possible since the HTTP handler construction is part of the websocket.Server.

This PR removes all http.Server setup:

  • The websocket.WsServer interface is modified to return an http.HandlerFunc to make it pluggable for any http.Handler
  • creating the http server or net listener becomes an application concern
  • TLS handling becomes a net.Listener concern and is no longer part of the OCPP library
  • websocket client and server are split into different files
  • dependency on gorilla/mux is eliminated

TODO

  • cleanup websocket interface descriptions
  • finalise websocket and network tests
  • repair any other test
  • add HTTP/TLS examples or wrapper functions

@lorenzodonini is this something you would consider? Waiting for your comments before spending further efforts.

@andig andig marked this pull request as draft October 7, 2023 12:53
@andig andig changed the title Split OCPP and HTTP server - Breaking Change Split WebSocket and HTTP server - Breaking Change Oct 7, 2023
@lorenzodonini
Copy link
Owner

Sorry in advance for the long post, but wanna address some points in detail.

First things first: have you tried using the wsServer.AddHttpHandler method? It allows registering arbitrary http handlers. The downside is that you will still need to effectively start/stop the http server using the lib. Not sure if this would work for you for the time being.

Regarding suggested approach

  • The websocket.WsServer interface is modified to return an http.HandlerFunc to make it pluggable for any http.Handler

I love it, but I would like to still offer devs the possibility to run this lib easily and standalone. I know creating an http server is only a few LOCs, but the UX of just having to invoke server.Start without worrying about routing is imho very appealing, especially if you're prototying.

That being said we could offer such a standalone version with an additional config parameter/function 🤔 This would also enable "backwards-compatibility".

  • creating the http server or net listener becomes an application concern

True, check my point above.

  • TLS handling becomes a net.Listener concern and is no longer part of the OCPP library

Same as above. Also tbf the entire TLS support was never meant to be used in large-scale systems (reverse proxies would take care of it) but rather for rapid prototyping.

  • websocket client and server are split into different files

Thank you, definitely a keeper 👍

  • dependency on gorilla/mux is eliminated

See my answer to the first point.

Assuming we offer both a standalone start option and your suggested approach, we would still need a simple HTTP server that supports pattern handling (/{ws}). IIRC http.ServeMux doesn't support this out-of-the-box, right? So we either use some lighweight router or write that logic ourselves.

@andig
Copy link
Contributor Author

andig commented Oct 9, 2023

Great answer, thank you! Sounds like you're ok with the goal/approach in general but would like to keep the ease of use for prototyping?

If that's correct we could create an example application (good Go practice) or provide some wrapper methods. Full compatibility would not be an option though. One thing that would get los- or rather no longer needed- might be testing of the wrapper methods, at least not to the extend done today.

First things first: have you tried using the wsServer.AddHttpHandler method

I didn't actually and it feels kinda backwards having to do this. Our application evcc.io starts an ocpp server only if configured and would then always need to do that just to serve it's UI. That's a bad dependency to have.

So we either use some lighweight router or write that logic ourselves.

Good point, agreed. I wouldn't care if mux is a dependency of this library tbo, just not having to use it would be great.

@lorenzodonini
Copy link
Owner

you're ok with the goal/approach in general but would like to keep the ease of use for prototyping?

Yes, that would be my wish.

If that's correct we could create an example application (good Go practice) or provide some wrapper methods.

Wrapper methods could indeed automatically create an HTTP router with all the existing logic and keep the dev experience basically identical. Updating the existing examples would then be a 1-liner. Happy to help with this part (although I currently have limited time to spare).

Our application evcc.io starts an ocpp server only if configured and would then always need to do that just to serve it's UI.

Makes sense. The suggested improvements are definitely welcome since everything remains stdlib-compatible.

@lorenzodonini
Copy link
Owner

Just fyi, since this will be a breaking change, I'd love to make it into a dedicated release when it's ready.

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.

Mix Gin Gonic and the Ocpp server under the same port
2 participants