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

jsonrpc: small refactor, extract method for groupedJSON-RPC calls. #71

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

msf
Copy link
Contributor

@msf msf commented Jul 29, 2024

this is a small improvment to the current ugly code we use for:

  • grouping error handling of rpc calls for the same block
  • abiding to a maximum concurrency limit for the whole RPC node.

This is ontop of #70,

@msf msf changed the base branch from main to add-tests-to-jsonrpc July 29, 2024 23:09
Comment on lines 117 to 143
func (c *rpcClient) GroupedJSONrpc(
ctx context.Context,
group *errgroup.Group,
method string,
args []any,
output *bytes.Buffer,
debugBlockNumber int64,
) {
group.Go(func() error {
errCh := make(chan error, 1)
c.wrkPool.Submit(func() {
defer close(errCh)
err := c.getResponseBody(ctx, method, args, output)
if err != nil {
c.log.Error("Failed to get response for jsonRPC",
"blockNumber", debugBlockNumber,
"method", method,
"error", err,
)
errCh <- err
} else {
errCh <- nil
}
})
return <-errCh
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering what you think of having a function like this:

func (c *rpcClient) execOnPoolInGroup(
	group *errgroup.Group,
	function func() error,
) {
	group.Go(func() error {
		errCh := make(chan error, 1)
		c.wrkPool.Submit(func() {
			defer close(errCh)
			err := function()
			if err != nil {
				errCh <- err
			} else {
				errCh <- nil
			}
		})
		return <-errCh
	})
}

That can be called like this:

c.execOnPoolInGroup(group, func() error {
	err := c.getResponseBody(ctx, method, args, output)
	if err != nil {
		c.log.Error("Failed to get response for jsonRPC",
			"blockNumber", debugBlockNumber,
			"method", method,
			"error", err,
		)
	}
	return err
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it, but don't we end up writting more core? we always want to call a jsonRPC method and including the err handling log.
Or said in a different way, I think that is a better abstraction to hide the mess of group.Go() + wrkPool.Submit() but I also want the syntatic sugar to run the prebaked closure you wrote above.

maybe: "porque no los dos?" -- we can have both, let me do that.

Base automatically changed from add-tests-to-jsonrpc to main July 30, 2024 14:22
extract a method for doing the grouped JSON-RPC calls
@msf msf merged commit bb12b10 into main Aug 22, 2024
1 check passed
@msf msf deleted the jsonrpc-grouped-rpcs branch August 22, 2024 21:00
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