xtra icon indicating copy to clipboard operation
xtra copied to clipboard

Introduce generic `ChanPtr`

Open thomaseizinger opened this issue 3 years ago • 9 comments

This introduces a generic ChanPtr type which adds reference counting to the channel.

This allows us to remove the Sender type and promote inbox::Receiver to a (future public) Mailbox type.

On most parts, the implementation is the same. Important differences are:

  • The actual logic for reference counting is encapsulated on Chan. This used to be #162 but I ended up doing it here because the merge conflicts would have been insane.
  • RefCounterPolicy does not have an into_either fn. This is solved by special-casing impl blocks for ChanPtr.
  • The new implementation is accompanied by a range of tests.
  • We remove the RefCounter trait from the public ref_count module. This is not actually necessary and allows us to avoid the sealing of the trait because users cannot name it anyway.

thomaseizinger avatar Jul 28 '22 18:07 thomaseizinger

If we end up merging https://github.com/Restioson/xtra/pull/160, I am gonna iterate on this design again. I think it would be a step backwards to introduce a "ref count policy" again just so we can use this abstraction also for the receiver side.

thomaseizinger avatar Jul 29 '22 12:07 thomaseizinger

One alternative idea to this would be to retain the Sender and Receiver newtypes but implement Deref on them so we don't need to forward all the methods.

thomaseizinger avatar Jul 30 '22 08:07 thomaseizinger

I think this will need to be rebased or updated now that #160 has been merged

Restioson avatar Aug 02 '22 09:08 Restioson

I think this will need to be rebased or updated now that #160 has been merged

Yep, I'll need to think about a new design here.

thomaseizinger avatar Aug 02 '22 14:08 thomaseizinger

So after thinking about this more, I think it is overall better to actually re-introduce a ref-count policy with this PR. Here is my thinking:

I am currently trying to achieve symmetry between various modules. #159 will introduce a top-level recv_future module that is equivalent to the send_future module. Both of these futures need to have a reference-counted pointer to the inner channel, i.e. they can't just take Arc<ChanA>>.

Address wraps a Sender which is technically not necessary, all of the logic of Sender can go into Address except that we would be introducing circular references between the address and the send_future module because SendFuture needs to have reference-counted pointer to Chan which - after the removal of Sender would be Address.

The same situation applies to Receiver. With #133, Receiver will be renamed to Mailbox or at least there will be a public Mailbox type with a receive function on it that will return a ReceiveFuture. Mailbox will thus be the equivalent of Address. ReceiveFuture will have to also contain a reference-counted pointer to the Chan which again, shouldn't be Mailbox itself because of the circular-reference issue.

Thus, I think despite an Rx reference count type not being strictly necessary, the ChanPtr abstraction is quite clean and provides a safe way for us to manage the reference counts to the channel. It can easily embedded in other types, most of the code can be reused between Address and Mailbox and the symmetry makes it easy to understand.

thomaseizinger avatar Aug 02 '22 18:08 thomaseizinger

Does the above sound reasonable to you @Restioson ?

thomaseizinger avatar Aug 04 '22 12:08 thomaseizinger

except that we would be introducing circular references between the address and the send_future module

Is this a problem? I don't think this will be a compile fail, anyway.

Is this essentially what you are imagining?

type Address<A, Rc> = ChanPtr<A, Rc>; // maybe with newtypes
type Mailbox<A> = ChanPtr<A, RxStrong>;

Then, the difference between RxStrong and TxStrong is simply which counter is modified.

Another way to look at it: yes, receiver has a ref counter, but this also encapsulates the logic for ref counting into ChanPtr rather than the receiver.

Restioson avatar Aug 05 '22 11:08 Restioson

except that we would be introducing circular references between the address and the send_future module

Is this a problem? I don't think this will be a compile fail, anyway.

It is not a technical problem but it is a design smell. With the right abstractions, you should be able to avoid circular dependencies and code is much easier understood if dependencies are uni-directional.

Is this essentially what you are imagining?

type Address<A, Rc> = ChanPtr<A, Rc>; // maybe with newtypes
type Mailbox<A> = ChanPtr<A, RxStrong>;

Then, the difference between RxStrong and TxStrong is simply which counter is modified.

This but with new-types so we have control over the public API of the library.

thomaseizinger avatar Aug 05 '22 15:08 thomaseizinger

Okay, this is ready for another review @Restioson! I've integrated #162 into here because the merge conflicts would have been insane.

I also updated the PR description to be an up-to-date summary of what this patch set does.

thomaseizinger avatar Aug 05 '22 22:08 thomaseizinger

All dependent PRs have been merged and feedback has been addressed :)

thomaseizinger avatar Aug 17 '22 07:08 thomaseizinger

Would appreciate another review here @Restioson :)

thomaseizinger avatar Aug 28 '22 18:08 thomaseizinger

I've left some comments - apologies for the long wait, I've been busy as term is coming to a close so we are writing tests and handing in assignments currently. It's vac soon so I should have more time then though :)

Restioson avatar Aug 30 '22 12:08 Restioson

I've left some comments - apologies for the long wait, I've been busy as term is coming to a close so we are writing tests and handing in assignments currently. It's vac soon so I should have more time then though :)

No worries and thanks for taking the time :)

thomaseizinger avatar Aug 30 '22 12:08 thomaseizinger

Good to merge once tests pass I think

Restioson avatar Sep 01 '22 12:09 Restioson

Good to merge once tests pass I think

Needs your approval though :)

thomaseizinger avatar Sep 01 '22 12:09 thomaseizinger