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

Distinguishing between errors #14

Open
matthewmueller opened this issue Jul 10, 2017 · 7 comments
Open

Distinguishing between errors #14

matthewmueller opened this issue Jul 10, 2017 · 7 comments

Comments

@matthewmueller
Copy link

matthewmueller commented Jul 10, 2017

Is it currently possible to distinguish between response errors and network errors in RPCC?

For example, with the DOM.getNodeForLocation method, it can sometimes return an error: No node found at given location. I don't really want to error out in all cases of this error, but for things like the RPCC websocket disconnecting, I would like to error out immediately.

Is there anyway to distinguish between these types of errors? Here's some (incorrect) psuedocode of what I'm trying to accomplish:

var args cdpcmd.DOMGetNodeForLocationArgs
if e := json.Unmarshal(req.Params, &args); e != nil {
	return raw, e
}
reply, err := c.DOM.GetNodeForLocation(ctx, &args)
if err != nil {
        if err.(*ResponseError) { 
           // handle differently
        } 
	return raw, err
}
return json.Marshal(reply)

Thanks!

@mafredri
Copy link
Owner

I haven't really found the need to differentiate between RPC errors and errors on the socket, as such I haven't put enough thought into how to deal with these kinds of situations.

One problem here is that relying on the RPC error tightly couples your code with the current behavior of the CDP protocol. What would you test for if you knew the Code/Message/Data of the RPC error? The error code returned by this function is -32000 with stands for a general Server error (according to the JSON-RPC 2.0 spec). This could mean anything. Also, the message (in this case No node found at given location) is only defined in the chromium/WebKit source and could easily have changed in a future release.

If we assume that the above is not an issue (e.g. we only want to differentiate RPC and socket errors) then technically this should be possible by doing something along the lines of (untested):

if rpcErr := cdp.ErrorCause(err); rpcErr != nil {
    if rpcErr, ok := rpcErr.(*rpcc.ResponseError); ok {
        // rpcErr.Code ?
    }
}

But I'm not very satisfied with this API. I'm thinking I would like to unexport ResponseError since it's very much an implementation detail. I'd much rather expose a different kind of API for this, maybe something like rpcc.StatusFromError(err). In turn, rpcc.Status could have the following methods: Code(), Message(), Data() (methods can allow for some decoupling via interfaces).

Since cdp errors implement interface causer { Cause() error }, we could test for this in StatusFromError so that we could simply do: if status := rpcc.StatusFromError(err); status != nil { /* do something with e.g. status.Code() */ }.

I haven't put a lot of thought into the naming above, I blatantly stole it from grpc-go/status/status.go.

Another option (to Status above) is exposing some new methods on rpcc, e.g. rpcc.ErrorCode(err), rpcc.ErrorMessage(err) and rpcc.ErrorData(err), although the last one's usefulness is questionable.

Thoughts / suggestions are welcome 😄.

@mafredri
Copy link
Owner

I just confirmed that the above works, more accurately, it might look like this:

_, err = c.DOM.GetNodeForLocation(ctx, dom.NewGetNodeForLocationArgs(x, y))
if err := cdp.ErrorCause(err); err != nil {
	if err, ok := err.(*rpcc.ResponseError); ok {
		log.Printf("Got error code: %d", err.Code)
	} else {
		return err
	}
}

Or a slightly longer version if you care about the original error:

if err != nil {
	if err2 := cdp.ErrorCause(err); err != err2 {
		if err2, ok := err2.(*rpcc.ResponseError); ok {
			log.Printf("Got error code: %d", err2.Code)
		} else {
			return err
		}
	} else {
		return err
	}
}

I'm still thinking about nicer solutions for this though 😉.

@mafredri
Copy link
Owner

Apologies for the spamming, I'm going to indulge my paranoia a bit 😏.

I couldn't remember how the CDP protocol deals with miscellaneous RPC errors, so I did the following test:

var args = struct {
	X string `json:"x"`
	Y int    `json:"y,omitempty"`
	Z int    `json:"z"`
}{
	X: "1000",
	Z: 1000,
}
err = rpcc.Invoke(ctx, "DOM.getNodeForLocation", &args, nil, conn)
fmt.Println(err)

Result:

rpc error: Invalid parameters (code = -32602, data = x: integer value expected; y: integer value expected)

So it does try to use predefined RPC errors where applicable (-32602 here). This makes it perhaps a bit more safe to rely on the RPC error code (e.g. 32000 returned when no node is found). Although, chromium seems to use this error code as a catch-all whenever there isn't a predefined RPC error. To really trust that we're not letting unknown errors pass by due to these kinds of checks, the only way I can think of is to actually test both error code and the message string (No node found at given location).

I worry that these errors could easily vary between browsers (we could be using Chrome, Safari or Edge, for example) or browser versions. Future versions of the protocol might introduce new errors for these functions that we might want to handle, etc.

Some food for thought.

@matthewmueller
Copy link
Author

matthewmueller commented Jul 13, 2017

@mafredri I apologize for the late response here and I really appreciate your hard work on this issue!

Sidenote: I noticed you use typescript on a few of your other open source projects, so I think you might be interested in browser client that I've been busy finishing up. Not quite ready yet, but I just invited you to the repo: https://github.com/matthewmueller/chromie. I modeled a lot of it after CDP's API design :-)

Back to the matter at hand! Yah I definitely agree that it would get hairy trying to make sense of the error codes and messages. The protocol changes so much. The motivation for this issue is that if the client hasn't fully loaded the page yet and you call DOM.getNodeForLocation on part of the blank page, you'll get an RPCC error.

I'd basically like to know which type of error I'm dealing with, not really the exact error:

  1. Is this an error with writing or reading from the socket?
  2. Is this a ResponseError from rpcc.Response. E.g.

    cdp/rpcc/conn.go

    Lines 250 to 260 in bcfae00

    // Response represents an RPC response or notification sent by the server.
    type Response struct {
    // RPC response to a Request.
    ID uint64 `json:"id"` // Echoes that of the Request.
    Result json.RawMessage `json:"result"` // Result from invokation, if any.
    Error *ResponseError `json:"error"` // Error, if any.
    // RPC notification from remote.
    Method string `json:"method"` // Method invokation requested by remote.
    Args json.RawMessage `json:"params"` // Method parameters, if any.
    }

If it's an error from reading or writing to the socket, you'd probably want to kill the program, if it's a user error, like trying to get a node before the page loads, I'd like to treat that differently, like show a warning

Is that possible?

@matthewmueller
Copy link
Author

matthewmueller commented Jul 13, 2017

Also in other news, I had no idea the chrome devtools protocol was coming to other platforms! that's awesome :-D

@mafredri
Copy link
Owner

I apologize for the late response here and I really appreciate your hard work on this issue!

No worries, I'm still thinking about how to solve this to provide the best ergonomics!

Back to the matter at hand! Yah I definitely agree that it would get hairy trying to make sense of the error codes and messages. The protocol changes so much. The motivation for this issue is that if the client hasn't fully loaded the page yet and you call DOM.getNodeForLocation on part of the blank page, you'll get an RPCC error.

Yeah, this seems like a solid use-case that should be supported (officially)!

Is that possible?

You can totally do it right now, check #14 (comment). But as I said, I'm still thinking of a better way to do this, so just a heads up that I might break it in the future 😄.

So to sum up: the *rpcc.ResponseError is what you're looking for. If it's not one of those, then it is (at least currently) either a rpcc.ErrConnClosing or an error that originates from gorilla/websocket or net or wherever.

The rpcc package doesn't wrap errors, only cdp (to ease with debugging) so you have the option to test for specific errors after checking the causer.

Sidenote: I noticed you use typescript on a few of your other open source projects, so I think you might be interested in browser client that I've been busy finishing up. Not quite ready yet, but I just invited you to the repo: matthewmueller/chromie. I modeled a lot of it after CDP's API design :-)

Oh, that's really cool, thanks for the invite! The API does look familiar, glad to see it has inspired you 😊. I hope I'll get around to contributing to it a well 🙂.


Also in other news, I had no idea the chrome devtools protocol was coming to other platforms! that's awesome :-D

Not exactly Chrome DevTools, but the protocol is (at least partially) implemented in other browsers as well: RemoteDebug Protocol Compatibility Tables. E.g. for Edge there's the edge-diagnostics-adapter (which is looking a bit stale atm). Beauty of CDP becomes most apparent when all browsers implement it (🤞).

@mafredri
Copy link
Owner

mafredri commented Feb 5, 2019

Following along with the Go 2 error proposal(s) to see what improvements we could do here. For one, an improvement that's immediately possible via the proposal (or right now the xerrors package):

var respErr *rpcc.ResponseError

reply, err := c.DOM.GetNodeForLocation(ctx, &args)
if err != nil {
        if xerrors.As(err, &respErr) {
                if respErr.Code == 30000 {
                        // handle differently
                }
        } 
	return raw, err
}

Which is pretty cool 😄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants