-
Notifications
You must be signed in to change notification settings - Fork 18
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
Lsps0 implementation #114
Lsps0 implementation #114
Conversation
For an example how to implement a service on top of this, see #115 |
e220db3
to
0783e40
Compare
f13d9d2
to
a6f2e33
Compare
Note: The CLN plugin can no longer be dynamic, because we're setting a feature bit now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Great!
The infrastructure and the layer separation looks promising to meet the evolving lsp spec requirements.
lsps0/server.go
Outdated
// NOTE: The handler is being called synchonously. There's an option to | ||
// do this in a goroutine instead. Also, there's the option to put the | ||
// goroutine in the method desc for specific methods instead. | ||
r, err := m.method.Handler(m.service.serviceImpl, context.TODO(), df) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it should be called in go routine otherwise the requests will block each other (in contrary to the grpc server behavior).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It now runs in a goroutine. On top of this, there's a mechanism to make sure there are not too many simultaneous requests.
s.RegisterService( | ||
&ServiceDesc{ | ||
ServiceName: "lsps0", | ||
HandlerType: (*ProtocolServer)(nil), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the HandlerType
? Where do we use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took it from the grpc service desc. It was used to check the runtime type of the handler passed via RegisterService
, whether it implements the interface in the service description. I had removed that check, because I misunderstood it. Reintroduced the check now. Let me know if you think that's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took it from the grpc service desc. It was used to check the runtime type of the handler passed via
RegisterService
, whether it implements the interface in the service description. I had removed that check, because I misunderstood it. Reintroduced the check now. Let me know if you think that's a good idea.
I see. As long as we have generic RegisterService underneath then I agree the check is a good idea.
I wonder if we can simplify and add the RegisterXXXServer methods (e.g RegisterProtocolServer) directly as a method of the Server struct and have that type safety check as part of the method prototype.
What do yo think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can simplify and add the RegisterXXXServer methods (e.g RegisterProtocolServer) directly as a method of the Server struct and have that type safety check as part of the method prototype.
I don't see a way to do that without creating a circular reference between the implementing services and the main server, do you have something in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see... I now understand that the interfaces are not declared in the base module (lsp0) but every addition introduces its own interface so indeed we can't do it the way I suggested.
|
Implement lsps0 server for CLN #104
grpc.NewServer
. You create anlsps0.NewServer()
. Then you register the required subservices, in this PR, it contains the ProtocolServer (see lsps0/protocol_server.go). The service has a service implementation and a service description. The service description maps the lsps0 method to the function being called (lsps0.listprotocols
). That way, the services can be focused on handling requests and responses, and the lsps0 server wraps these in jsonrpc 2.0 messages over lightning custom messages.Note: This PR points to the
lsps2
branch. The idea is to create a feature branch with lsps2 changes.Order of pull requests: