Add option to use packed encoding in rpc.StreamTransport.

[lthibault]

This PR allows users to specify the encoding (packed vs unpacked) used by rpc.StreamTransport.

It achieves this by adding the rpc.StreamTransportOption type to parametrize rpc.StreamTransport, and the rpc.WithCodec option. Codec is a factory type that can encode/decode messages to/from bytes.

In the future, we should perhaps look into reusing capnp.Encoder and capnp.Decoder, for example with a sync.Pool manipulated by codec types.

I have not tested this beyond ensuring the current unit tests continue to pass.

⏱️ Estimated review time: 10 mins.

[zombiezen]

Thanks for the short, focused PR. This helps a lot. 👍

Sending a packed stream over the network does make sense to me as a feature, but can you help me understand why we want to give the user a general mechanism for overriding how messages get read and written? Particularly, could we give another constructor that has packing behavior instead?

My experience from working on RPC packages is that the more callback hooks you give the user, the trickier it is to reason about the locking, because you have to be extraordinarily careful to release the lock while the user code is running or document precisely what the user is allowed to do in the callback. Otherwise, the user can have hard-to-reason-about deadlocks or inconsistent behavior.

[lthibault]

Thanks for the short, focused PR. This helps a lot. 👍

My pleasure -- happy it helps!

Sending a packed stream over the network does make sense to me as a feature, but can you help me understand why we want to give the user a general mechanism for overriding how messages get read and written? Particularly, could we give another constructor that has packing behavior instead?

I have no argument against providing NewPackedStreamTransport. The decisions here were mostly made for the sake of expediency and fast feedback.

In the back of my mind, I was thinking about a future PR in which we could pool encoders and decoders, and my thinking was that the get/put logic could be contained in Codec.

general mechanism for overriding how messages get read and written

Just to make sure we're on the same page, is this referring to the use of options, or the use of a Codec interface? I'm not sure I understand how this qualifies as a general mechanism since option types work by manipulating private fields (i.e. no surface for uninformed users to misconfigure) and Codec doesn't provide access to anything that needs synchronization.

Again, I'm totally happy to just provide a new constructor -- I just want to sync our mental models in order to make better-informed design decisions in the future 🙂

My experience from working on RPC packages is that the more callback hooks you give the user, the trickier it is to reason about the locking, because you have to be extraordinarily careful to release the lock while the user code is running or document precisely what the user is allowed to do in the callback. Otherwise, the user can have hard-to-reason-about deadlocks or inconsistent behavior.

This makes sense in general, but as mentioned above, it seems like the present changes don't touch anything that needs locking? I'm guessing you're concerned about what kinds of future options might get added?

In any case, thanks for the quick feedback. I'm going to make the requested changes and push again in a few minutes.

[zenhack]

I made a specific comment re: the Codec interface in-line. I might suggest instead doing something like:

interface Codec {
    Encode(m *capnp.Message) error
    Decode() (*capnp.Message, error)
    Close() error
}

type codec struct {
    *Encoder
    *Decoder
    io.Closer
}

func NewCodecTransport(codec Codec) Transport { /* ... */ }

func NewPackedStreamTransport(rwc io.ReadWriteCloser) Transport {
    return NewCodecTransport(codec{
        Encoder: NewPackedEncoder(rwc),
        Decoder: NewPackedDecoder(rwc),
        Closer: rwc,
    })
}

func NewStreamTransport(rwc io.ReadWriteCloser) Transport {
    // Similar to NewPackedStreamTransport
}

(with possible name bikeshedding)

[zenhack]

Quoting Louis Thibault (2021-02-13 14:26:50)

@zenhack I'm not sure how the codec implementation you propose would interact with ctxReader. Any thoughts?

My first instinct is that the Codec should probably just take the context itself, but it seems like reworking the APIs might be a bit too much yak shaving for this feature for now; I think @zombiezen is right, let's just expose NewPackedStreamTransport() for now and we can generalize later if desired.

[zombiezen]

Replying to @lthibault:

Again, I'm totally happy to just provide a new constructor -- I just want to sync our mental models in order to make better-informed design decisions in the future 🙂

I'm 100% on the same page: I'm intentionally being a little chattier than I normally am on code review so that I can share mindset.

This makes sense in general, but as mentioned above, it seems like the present changes don't touch anything that needs locking? I'm guessing you're concerned about what kinds of future options might get added?

Ah right, I guess we already release the lock before I/O operations for exactly this reason.

---

BTW, we might want to switch the CI from Travis to GitHub Actions. The Bazel builds should probably be dropped, and the Go version needs to be updated.

[lthibault]

BTW, we might want to switch the CI from Travis to GitHub Actions. The Bazel builds should probably be dropped, and the Go version needs to be updated.

Glad to hear you say it -- I was wondering how to diplomatically bring this up! 😆

[lthibault]

@zombiezen @zenhack

I think I've found an approach that satisfies Ian's remarks.

Test for the rpc package pass. I've added a new test for the packed encoding.

[lthibault]

@zenhack @zombiezen You guys good to merge this?

[zenhack]

Works for me.