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

fix: make resp result/error conformant to spec #107

Merged
merged 2 commits into from
May 8, 2024

Conversation

LesnyRumcajs
Copy link
Contributor

Closes #102

This is a more verbose version of #103, mainly because my Go-Fu is too weak to find the issue in that code that causes timeouts. Are there any performance regressions with this? How can I check it?

It works fine on almost latest master, was 5c010e6 a regression @magik6k?

❯ go test -count=1 -v ./... | tail
--- PASS: TestReverseCallDroppedConn (1.10s)
=== RUN   TestBigResult
    rpc_test.go:1607: skipping test due to requiced resources, set I_HAVE_A_LOT_OF_MEMORY_AND_TIME=1 to run
--- SKIP: TestBigResult (0.00s)
PASS
ok      github.com/filecoin-project/go-jsonrpc  7.408s
=== RUN   TestReaderProxy
--- PASS: TestReaderProxy (0.00s)
PASS
ok      github.com/filecoin-project/go-jsonrpc/httpio   0.352s

@rvagg
Copy link
Member

rvagg commented May 8, 2024

Yes, it was a regression. I've opened an alternative at #108 which should deal with the original reported problem and let through jsonrpc native errors.

@rvagg
Copy link
Member

rvagg commented May 8, 2024

I think the original regression was noted with the length of time it takes to run tests, so:

On #108 so that it actually passes:

$ time go test ./... -count 1
?       github.com/filecoin-project/go-jsonrpc/auth     [no test files]
?       github.com/filecoin-project/go-jsonrpc/metrics  [no test files]
ok      github.com/filecoin-project/go-jsonrpc  6.771s
ok      github.com/filecoin-project/go-jsonrpc/httpio   0.004s

real    0m7.092s
user    0m4.143s
sys     0m2.457s

And on your branch, with #108 cherry-picked:

$ time go test ./... -count 1
?       github.com/filecoin-project/go-jsonrpc/auth     [no test files]
?       github.com/filecoin-project/go-jsonrpc/metrics  [no test files]
ok      github.com/filecoin-project/go-jsonrpc  6.785s
ok      github.com/filecoin-project/go-jsonrpc/httpio   0.004s

real    0m7.094s
user    0m4.051s
sys     0m2.514s

That looks pretty consistent to me.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems OK to me; an alternative, more terse form might be:

func (r response) MarshalJSON() ([]byte, error) {
	data := make(map[string]interface{})
	data["jsonrpc"] = r.Jsonrpc
	data["id"] = r.ID
	if r.Error != nil {
		data["error"] = r.Error
	} else {
		data["result"] = r.Result
	}
	return json.Marshal(data)
}

Co-authored-by: Rod Vagg <[email protected]>
@LesnyRumcajs
Copy link
Contributor Author

this seems OK to me; an alternative, more terse form might be:

func (r response) MarshalJSON() ([]byte, error) {
	data := make(map[string]interface{})
	data["jsonrpc"] = r.Jsonrpc
	data["id"] = r.ID
	if r.Error != nil {
		data["error"] = r.Error
	} else {
		data["result"] = r.Result
	}
	return json.Marshal(data)
}

Less explicit, but more readable, I like it. Changed and co-authored you. Thanks!

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and tests appear to be happy with the latest master. Thanks!

@magik6k magik6k merged commit 2acf375 into filecoin-project:master May 8, 2024
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.

Methods that return nothing must contain result field
3 participants