rpc: implement full bi-directional communication #18471
Conversation
| ) | ||
|
|
||
| const ( | ||
| jsonrpcVersion = "2.0" | ||
| vsn = "2.0" |
There was a problem hiding this comment.
Three letter abbreviation of version, really? Please use version or leave it as it was previously
There was a problem hiding this comment.
IMHO the constant name has to be shorter than "2.0", otherwise it's not
worth defining it.
There was a problem hiding this comment.
I will change it to version.
|
Looks like the last commit finallly fixed the websocket issue. |
|
Finally support for Also, regarding parse errors, this still seems to work: But basically, these broken json requests change (master below) This PR: I'd reckon that most errors are not on the |
|
This is odd. The first one is against It doesn't allow There's still no response? |
|
Ah, there's something racey here.. |
|
There appears to be two separate clients reading from the same source, and one of them reads the message, the other hits an |
|
Hm, the one from |
|
Oddly, even if I remove that whole chunk of self-attach code there's still some race. The only real difference is that I don't notice it until later, when the request comes in (if I leave the chunk in, it becomes triggered earlier) However, it appears that ServeCodec somehow spins up two cliens, or alternatively invokes the dispatcher twice, not quite sure. |
|
Ok, I think I found it. In yields So the Not sure about the correct way to fix that... |
|
This fixes the problem above, but the fix is not complete. It doesn't handle batch messages, and I'm not sure how it deals with subscriptions. diff --git a/rpc/client.go b/rpc/client.go
index 6ca5b3c6c..9fc327a94 100644
--- a/rpc/client.go
+++ b/rpc/client.go
@@ -22,6 +22,7 @@ import (
"encoding/json"
"errors"
"fmt"
+ "io"
"net/url"
"reflect"
"strconv"
@@ -533,10 +534,9 @@ func (c *Client) dispatch(codec ServerCodec) {
}
close(c.didClose)
}()
-
// Spawn the initial read loop.
go c.read(codec)
-
+ responseDoneCh := make(chan (bool), 1)
for {
select {
case <-c.close:
@@ -547,10 +547,16 @@ func (c *Client) dispatch(codec ServerCodec) {
if op.batch {
conn.handler.handleBatch(op.msgs)
} else {
- conn.handler.handleMsg(op.msgs[0])
+ conn.handler.handleMsg(op.msgs[0], responseDoneCh)
}
case err := <-c.readErr:
+ if err == io.EOF {
+ // No more reading, but might not want to close it immediately, in case
+ // we are still processing and wants to write data.
+ <- responseDoneCh
+
+ }
conn.handler.log.Debug("RPC connection read error", "err", err)
conn.close(err, lastOp)
reading = false
diff --git a/rpc/handler.go b/rpc/handler.go
index bc03ef25f..44a6c2d73 100644
--- a/rpc/handler.go
+++ b/rpc/handler.go
@@ -131,7 +131,7 @@ func (h *handler) handleBatch(msgs []*jsonrpcMessage) {
}
// handleMsg handles a single message.
-func (h *handler) handleMsg(msg *jsonrpcMessage) {
+func (h *handler) handleMsg(msg *jsonrpcMessage, done chan(bool)) {
if ok := h.handleImmediate(msg); ok {
return
}
@@ -144,6 +144,9 @@ func (h *handler) handleMsg(msg *jsonrpcMessage) {
for _, n := range cp.notifiers {
n.activate()
}
+ if done != nil{
+ done <- true
+ }
})
}
diff --git a/rpc/server.go b/rpc/server.go
index 5a92847f2..75b564743 100644
--- a/rpc/server.go
+++ b/rpc/server.go
@@ -111,7 +111,7 @@ func (s *Server) serveSingleRequest(ctx context.Context, codec ServerCodec) {
if batch {
h.handleBatch(reqs)
} else {
- h.handleMsg(reqs[0])
+ h.handleMsg(reqs[0], nil)
}
}
|
|
Thank you for the investigation! I'll fix it. |
|
Fix committed. The issue with |
|
@holiman please try your example again, somehow it doesn't work for me. I said it's fixed because the unit test I added passes now but maybe it's different with unix sockets and real-life |
|
I'm still thinking about adding the parse errors back in. |
|
Yup, the fix works for me. If you don't add parse errors back in, I'd suggest to instead bump the error message from Otherwise, people will just have a hard time debugging their cross-platform woes, and they'll bug us about it. |
New APIs added:
client.RegisterName(namespace, service) // makes service available to server
client.Notify(ctx, method, args...) // sends a notification
ClientFromContext(ctx) // to get a client in handler method
This is essentially a rewrite of the server-side code. JSON-RPC
processing code is now the same on both server and client side. Many
minor issues were fixed in the process and there is a new test suite for
JSON-RPC spec compliance (and non-compliance in some cases).
List of behavior changes:
- Method handlers are now called with a per-request context instead of a
per-connection context. The context is canceled right after the method
returns.
- Subscription error channels are always closed when the connection
ends. There is no need to also wait on the Notifier's Closed channel
to detect whether the subscription has ended.
- Parse errors just close the connection instead of reporting an error
back to the client, except on HTTP connections. I'm not happy about
this change, but it was non-trivial to implement this within the
concurrency model used in Client.
- Client now omits "params" instead of sending "params": null when there
are no arguments to a call. The previous behavior was not compliant
with the spec. The server still accepts "params": null.
- Floating point numbers are allowed as "id". The spec doesn't allow
them, but we handle request "id" as json.RawMessage and guarantee that
the same number will be sent back.
- Logging is improved significantly. There is now a message at DEBUG
level for each RPC call served.
Read operations could be dropped after reconnect because the new reader goroutine would be started before the previous one exited. Start it later. For short-lived connections performing a single call, the result would sometimes be dropped because we closed the connection before waiting for handler shutdown. Reverse shutdown order to fix this problem.
|
rebased and parse errors are back |
|
In case it's interesting to anyone involved in this, I also wrote a bidirectional jsonrpc2 implementation in Vipnode last August:
Let me know if there's desire to collaborate in the future. |
New APIs added:
This is essentially a rewrite of the server-side code. JSON-RPC
processing code is now the same on both server and client side. Many
minor issues were fixed in the process and there is a new test suite for
JSON-RPC spec compliance (and non-compliance in some cases).
List of behavior changes:
per-connection context. The context is canceled right after the method
returns.
ends. There is no need to also wait on the Notifier's Closed channel
to detect whether the subscription has ended.
are no arguments to a call. The previous behavior was not compliant
with the spec. The server still accepts "params": null.
them, but we handle request "id" as json.RawMessage and guarantee that
the same number will be sent back.
level for each RPC call served.
Supersedes #18075 (this PR implements #18075 (comment))
Supersedes #18078 (suitableCallbacks is much simplifed)