don't use the context to shut down the relay

[marten-seemann]

Not a big fan of this change. I'd have preferred to save the AutoRelay on the BasicHost, but that's not possible (import cycle).

[vyzo]

I don't like this proliferation of host wrappers either -- can't we just save the close function in the host and invoke that?
We can even compose multiple close functions into one this way for all dependent services.

[marten-seemann]

I thought of that, but I’m not sure if having a SetClose(closeFunc) method on the Host is nicer.

[vyzo]

we dont have to expose it in the interface; the constructor can just collect what it needs. It can also be a slice of io.Closers that we store.

…

On Fri, Sep 10, 2021, 13:56 Marten Seemann ***@***.***> wrote: I thought of that, but I’m not sure if having a SetClose(closeFunc) method on the Host is nicer. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1184 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SSZKIZVIFIL2WL5DS3UBHP55ANCNFSM5DZBONQQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[vyzo]

Another, more convoluted abstraction is to track services by unique keys and expose interface methods for managing that. This can also eliminate atrocities like the basic host cast in lotus just to get to autonat. But thats probably a more general discussion to be held here.

…

On Fri, Sep 10, 2021, 14:06 Dimitris Vyzovitis ***@***.***> wrote: we dont have to expose it in the interface; the constructor can just collect what it needs. It can also be a slice of io.Closers that we store. On Fri, Sep 10, 2021, 13:56 Marten Seemann ***@***.***> wrote: > I thought of that, but I’m not sure if having a SetClose(closeFunc) > method on the Host is nicer. > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > <#1184 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAI4SSZKIZVIFIL2WL5DS3UBHP55ANCNFSM5DZBONQQ> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > >

[marten-seemann]

We can’t put it in the constructor, as the autonat depends on the host.

Let’s keep wrapping for now, I think that’s the more idiomatic way to handle this in Go anyway.

[vyzo]

I meant in the libp2p constructor, *not* the object constructor; i'd rather avoid the wrapping.

…

On Fri, Sep 10, 2021, 15:06 Marten Seemann ***@***.***> wrote: We can’t put it in the constructor, as the autonat depends on the host. Let’s keep wrapping for now, I think that’s the more idiomatic way to handle this in Go anyway. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1184 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SVAYTHHXEHZOB63C6DUBHYC7ANCNFSM5DZBONQQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[vyzo]

actually I'll give the blessing; I am allergic to host wrapping, but on the same time I can't think of any really good reason not to wrap, so let's do it.

We should have the discussion about services at some point, so that we can eliminate wrapping altogether in the future.

[Stebalien]

Can I not pass in a set of abstract "service" constructors into the host? Of the form func(Host) (io.Closer, error)?

(note: fx/dependency injection would also fix this, we really need a way to "manage" libp2p services).

[marten-seemann]

Can I not pass in a set of abstract "service" constructors into the host? Of the form func(Host) (io.Closer, error)?

I'd love to do this. We just need to figure out a way to pass options to that constructor.