Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The closableBasicHost type needs to be exported so users can access the underlying BasicHost #2809

Closed
aarshkshah1992 opened this issue May 22, 2024 · 5 comments

Comments

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented May 22, 2024

  • Looks like the libp2p.New function was changed to replace the exported BasicHost type with an un-exported closableBasicHost type that wraps a BasicHost

return &closableBasicHost{App: app, BasicHost: bh}, nil

  • However, there is code in Lotus that depends on being able to access the concrete BasicHost implementation from a libp2p Host and that breaks now.

Please can we export the closableBasicHost type ?

For reference, the Lotus code that breaks is at https://github.com/filecoin-project/lotus/blob/967524aa83852123206715d0f54fd552c7546d5b/node/impl/net/net.go#L138.

@aschmahmann
Copy link
Collaborator

@aarshkshah1992 in the specific case you raised can't you get around this by doing an interface check for GetAutoNat rather than a type check?

Maybe the underlying type should be exported, but it seems more likely that it's just a symptom of the more important need of getting access to some of the optional (if common) components of the Host.

@aarshkshah1992
Copy link
Contributor Author

@aschmahmann Yeah, you are right. We don't need this change for Lotus anymore. Please feel free to close the issue.

@MarcoPolo
Copy link
Collaborator

I'm definitely open to exporting this type, but I'd like to see a better use case. If other folks have use cases or things that broke, please let me know.

It's not as simple as just exporting the type and changing what this function returns because it can now return either a closableBasicHost or a closableRoutedHost.

And generally I want to move away from BasicHost actually being a KitchenSinkHost. I want to move towards services being more pluggable (#1993).

@MarcoPolo
Copy link
Collaborator

After chatting with @sukunrt for a bit, we think (if needed) we could flip the current style and have basic host call app.Close when it closes. That way we wouldn't need to export a new type and could keep exporting basic host.

Writing this for posterity, but it seems like it may not be needed.

@sukunrt
Copy link
Member

sukunrt commented Sep 18, 2024

@aarshkshah1992 can you use EvtLocalReachabilityChanged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants