Suggestion: A better API for `RPC` method in Go

[avinassh]

I used the async RPC method in the broadcast challenge. One of the issues I faced was keeping track of the status of calls.

I wanted to maintain a list of messages I had sent. If the node had sent the message successfully, I would delete it from the list. If not, I would retry again.

Once I sent a message, I wanted to know if I had gotten any response or if the call had failed. The RPC method adds the auto-incremented message-id, but it does not return to the caller, but the returned status contains the message id. So it was painful to manage this.

Here is what I did:

1. Generate a random ID and add this to HandlerFunc's state (the callback method)
2. Send the message, save the ID in my local state
3. When the callback is called, remove the ID from the state
4. After X minutes, if my ID is still in the state, then resend the message

Sample code:

// this function gives a stateful HandlerFunc 
// which stores the msgId with it
//
// when this method is called, then it knows for which
// message it was called
rpcHandler := func(msgId string) maelstrom.HandlerFunc {
	return func(msg maelstrom.Message) error {
		// do something with the msgId
                // store.Remove(msgId)
		return nil
	}
}

// generate a msgId for this RPC call
msgId, _ := uuid.NewRandom()
// store this msgId in a map
// store.Save(msgId)
if err := n.RPC(nodId, msgBody, rpcHandler(msgId.String())); err != nil {
	return false
}

We can improve this by a great deal by making the RPC method return the msgId of the message it had sent. The change in the go library is minimal, but for the user, the code would be something like the following:

// the handler function does not require to be stateful anymore
someHandlerFunc := func(msg maelstrom.Message) error {
	var body map[string]any
	if err := json.Unmarshal(msg.Body, &body); err != nil {
		return err
	}
	// body: map[in_reply_to:1 type:broadcast_ok]
	if msgId, ok := body["in_reply_to"]; ok {
		// do something with the msgId
		// store.Remove(msgId)
	}
	return nil
}


if msgId, err := n.RPC(nodId, msgBody, someHandlerFunc); err != nil {
	return false
}
// store this msgId in a map
// store.Save(msgId)

The change in the library:

- func (n *Node) RPC(dest string, body any, handler HandlerFunc) error { ... }
+ func (n *Node) RPC(dest string, body any, handler HandlerFunc) (int, error) { ... }

It is a breaking change, but the developer experience this change provides makes it worth it. Are you open to considering such a change?

[aphyr]

Ah, I'm not really qualified to speak on this--perhaps @benbjohnson might?

[benbjohnson]

@avinassh It's probably a better API to return the message ID, however, you shouldn't need to do message ID tracking for any of the challenges. I think breaking the API will be significantly more painful than it's worth. For that reason, I'd recommend not implementing the change.

[avinassh]

It's probably a better API to return the message ID, however, you shouldn't need to do message ID tracking for any of the challenges.

Continuing from the previous example, I had a background worker check the store for old messages:

go func() {
	ticker := time.NewTicker(100 * time.Millisecond)
	for range ticker.C {
		randomMsgId := store.Get()
		// randomMsgId is not sent, so resend again
	}
}()

In the above snippet, msgId is randomly generated, where I kept the state. Without message ids, this is painful.

Though I agree that tracking message Ids is not required for any challenges, but when I was solving broadcast challenge, this is how I started working on the solution, it took me a while to realise this is not required at all and later removed the tracking of message ids. So, if someone else were to try the same, I thought we could make it easier.

I think breaking the API will be significantly more painful than it's worth. For that reason, I'd recommend not implementing the change.

I am a bit conflicted about this. The way I am using this library is to initiate the go mod once and then implement the challenges. I assume most people might do the same and not update the library when working on a challenge, but I could also be wrong. But when I think from the point of view of future users, this breaking change is okay as it will improve their experiences.