• Need to verify that these are safe before merge – the tail worker one is not but can be removed after a different in-review change.

Maybe not as common, but does this miss RefcountedWrapper

Maybe not as common, but does this miss RefcountedWrapper

No, that should already be covered – RefcountedWrapper inherits from kj::Refcounted and does not define any attach methods on its own.

Maybe not as common, but does this miss RefcountedWrapper

No, that should already be covered – RefcountedWrapper inherits from kj::Refcounted and does not define any attach methods on its own.

RefcountedWrapper does, but the underlying type is what's checked.

Maybe not as common, but does this miss RefcountedWrapper

No, that should already be covered – RefcountedWrapper inherits from kj::Refcounted and does not define any attach methods on its own.

RefcountedWrapper does, but the underlying type is what's checked.

Maybe I'm missing something, but I think it works just fine:
If we construct a RefcountedWrapper like auto rcClient = kj::refcounted<RefcountedWrapper>(kj::mv(client)); (example taken from kv.c++), then rcClient.attach(kj::mv(abc)) will produce the deprecation warning since RefcountedWrapper is a refcounted type. If we were to do an attachment before constructing RefcountedWrapper (i.e. assume the non-refcounted client already had something attached to it), that is fine too since the lifetime of the attachment is tied to when client is deallocated which is only when all instances of the RefcountedWrapper have been deallocated, so the lifetime does not depend on that of a single reference.

wrapper->addWrappedRef().attach(dep) isn't caught

wrapper->addWrappedRef().attach(dep) isn't caught

That function call would not result in a deprecation warning but I think that's just fine, that would not leave us vulnerable to attachment lifetime being shorter than expected: The lifetime of the attachment is tied to when the wrapped variable is deallocated which is only when all instances of the RefcountedWrapper have been deallocated, so the lifetime does not depend on that of a single instance of the wrapper.

The lifetime of the attachment is tied to when the wrapped variable is deallocated which is only when all instances of the RefcountedWrapper have been deallocated, so the lifetime does not depend on that of a single instance of the wrapper.

No. The attachment is tied to the Own<T> returned by addWrappedRef(), not to the wrapped variable. When that specific Own<T> is destroyed, the attachment is destroyed - even if other references from addWrappedRef() still exist.

It's unclear that there's a legitimate use case for attachToThisReference that shouldn't be address by other means. When is it better to tie a lifetime to an arbitrary reference lifetime?

It ends up being "this was the easy way to make it compile"

It's unclear that there's a legitimate use case for attachToThisReference that shouldn't be address by other means. When is it better to tie a lifetime to an arbitrary reference lifetime?

It ends up being "this was the easy way to make it compile"

Here's two use cases where we'd have to come up with a replacement for attachToThisReference():

• Let's say we need to attach to an instance of a class that never actually has addRef() called on it, but inherits from a class that inherits from Refcounted (so we can't get rid of the refcounting support even though we don't use it). In that case we'll get the deprecation warning for attach() despite never using it.
• In the downstream PR, we use attachToThisReference() in several places where attaching to a single refcounted instance is deliberate: We want to decrement a counter once this reference has been deallocated and not when the underlying object is decremented, so attachToThisReference covers exactly what we want.

If we want to not use attachToThisReference(), we'll need an alternative approach for both of these use cases.

It's unclear that there's a legitimate use case for attachToThisReference that shouldn't be address by other means.

@jasnell could you strategically advise here? We're not fully locking down this bad pattern by allowing attachToThisReference and not covering RefcountedWrapper. I'm wondering how feasible it is to try to remove the pattern entirely, or lock in this which partially helps guard against future usage.

I haven't read the full PR and don't have complete context. What are the expected semantics of attachToThisReference?

In general, I've never really liked how attach(...) works even with regular kj::Owns. I'd be much happier if the attachments were to the underlying T and not on the kj::Own<T>.

I haven't read the full PR and don't have complete context. What are the expected semantics of attachToThisReference?

It bypasses the protection from attaching to refcounted objects added here. @kentonv raised the concern behind this change.

I'd be much happier if the attachments were to the underlying T and not on the kj::Own.

How should we go about this?

... How should we go about this?

I don't believe there's a way to do it systematically. It would have to be on a case by case basis, with each individual type. Essentially, all I mean by the comment is that I rather wish we'd get away from attachments on the smart-pointers and instead rely on attachments on the actual objects, which is more of a general style change.

Unresolved questions on the validity of the approach

If you're not convinced that this approach is valid, can you suggest an alternative approach?

I think for the sake of making progress, an incremental approach should be fine. Absent a concrete alternative now, we can go with attachToThisReference so long as it doesn't introduce it's own issues, then iterate from there to handling the lifecycles more directly.

The concrete alternative I prefer to see is to remove attachToThisReference and ban the pattern

Yeah, I get that. However, for the couple of places it's used, are you certain the lifecycles of the objects will allow for that? If not, the "concrete suggestions" I would prefer to see would be the changes that ensure the lifecycle is correct, if any are needed.

I agree with James here – in some cases it's hard to avoid. Mar, did you consider the two use cases for attachToThisReference() I described above?

However, for the couple of places it's used, are you certain the lifecycles of the objects will allow for that?

My suspicion is they're all possible to rework. I pulled you in to confirm my intuition, and I will lift my objection if you feel it's unavoidable. To me, doesn't seem so.

Mar, did you consider the two use cases for attachToThisReference() I described above?

Is the first concern theoretical or real case somewhere? That seems like a case on unintentional inheritance.
Re the second: I spot-checked the internal PR (see comment). The author should investigate the rest.

It's likely possible to rework. I think the more important question is whether it's necessary to block this to wait on that. Some effort to determine what reworking those would look like and the extent of the changes would be needed. I'd say it's likely good to pursue as a follow up.

It's likely possible to rework. I think the more important question is whether it's necessary to block this to wait on that. Some effort to determine what reworking those would look like and the extent of the changes would be needed. I'd say it's likely good to pursue as a follow up.

SG. Given that we've already found one bug, we should evaluate each use of attachToThisReference carefully.