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

MoleClient: handle unknown error codes #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yoursunny
Copy link

The code has a bug: if the server sends an error code other than -32601 and -32002, the client shows an unfriendly error: errorBuilder is not a function.

$ node ./main.js 
C:\Users\sunny\Documents\code\node-mole-rpc\MoleClient.js:197
        return errorBuilder(errorData);
               ^

TypeError: errorBuilder is not a function
    at MoleClient._makeErrorObject (C:\Users\sunny\Documents\code\node-mole-rpc\MoleClient.js:197:16)
    at MoleClient._processSingleCallResponse (C:\Users\sunny\Documents\code\node-mole-rpc\MoleClient.js:106:38)
    at MoleClient._processResponse (C:\Users\sunny\Documents\code\node-mole-rpc\MoleClient.js:88:18)
    at WebSocket.callback (C:\Users\sunny\Documents\code\mole-test\node_modules\←[4mmole-rpc-transport-ws←[24m\TransportClientWS.js:24:13)
    at WebSocket.onMessage (C:\Users\sunny\Documents\code\mole-test\node_modules\←[4mws←[24m\lib\event-target.js:125:16)  
←[90m    at WebSocket.emit (events.js:315:20)←[39m
    at Receiver.receiverOnMessage (C:\Users\sunny\Documents\code\mole-test\node_modules\←[4mws←[24m\lib\websocket.js:800:20)
←[90m    at Receiver.emit (events.js:315:20)←[39m
    at Receiver.dataMessage (C:\Users\sunny\Documents\code\mole-test\node_modules\←[4mws←[24m\lib\receiver.js:436:14)     
    at Receiver.getData (C:\Users\sunny\Documents\code\mole-test\node_modules\←[4mws←[24m\lib\receiver.js:366:17)

This patch fixes the bug by constructing a generic Error object with errorData.code and errorData.message.


To reproduce the bug:

client.js

const MoleClient = require('mole-rpc/MoleClient');
const TransportClientWS = require('mole-rpc-transport-ws/TransportClientWS');
const WebSocket = require('ws');

async function main() {
  const client = new MoleClient({
    requestTimeout: 1000,
    transport: new TransportClientWS({
      wsBuilder: () => new WebSocket(`ws://192.168.5.13:7000`, { protocol: "text" })
    })
  });

  try {
    const arg = {};
    const result = await client.callMethod("X.CreateError", arg);
    console.log(result);
  } catch (err) {
    console.error(err);
  }
}

main().then(console.log, console.error);

server.go

package main

import (
	"context"
	"errors"
	"log"
	"net/http"
	"net/rpc"

	"github.com/powerman/rpc-codec/jsonrpc2"
	"nhooyr.io/websocket"
)

type X struct{}

func (X) CreateError(args struct{}, res *int) error {
	return errors.New("server error")
}

func main() {
	rpc.Register(X{})

	fn := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		c, err := websocket.Accept(w, r, nil)
		if err != nil {
			log.Println(err)
			return
		}
		defer c.Close(websocket.StatusInternalError, "")
		jsonrpc2.ServeConn(websocket.NetConn(context.Background(), c, websocket.MessageText))
		c.Close(websocket.StatusNormalClosure, "")
	})

	err := http.ListenAndServe("0.0.0.0:7000", fn)
	log.Fatal(err)
}

Response sent by server:

{"jsonrpc":"2.0","id":"AayLg-OHP0","error":{"code":-32000,"message":"server error"}}

@yoursunny
Copy link
Author

Hey @koorchik can you review and merge?

@yoursunny
Copy link
Author

After a long time without action from the repository owner, I have decided to publish my patches in a fork: @yoursunny/mole-rpc.
If anyone else is affected from this bug, you can try my fork.

@justin0mcateer
Copy link

This issue will affect our ability to use as well. I've just been reviewing the code for our use and there are several 'robustness' issues that stick out, this being one of them....

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.

2 participants