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

rcmgr: Backwards compatibility if you wrap default impl #2805

Merged
merged 3 commits into from
May 23, 2024

Conversation

MarcoPolo
Copy link
Collaborator

I tried to introduce a new method to the ResourceManager without breaking the API. That was a mistake, I should have broken the API because the new method was effectively required.

Users who wrapped the default implementation of the ResourceManager wouldn't get the new method. The circuitv2 transport would not see the new method, and then call the fallback method, which would fail in the default implementation. This is a subtle footgun that could have been avoided by either changing the api or changing the default implementation's fallback method to know not to expect an IP address when doing a circuitv2 connection. This fix does the latter for now to not break the API. Another PR I'll introduce soon will break the API and I'll try to merge that before the next release.

@MarcoPolo MarcoPolo requested a review from sukunrt May 21, 2024 19:00
@@ -324,6 +324,22 @@ func (r *resourceManager) OpenConnectionNoIP(dir network.Direction, usefd bool,
}

func (r *resourceManager) OpenConnection(dir network.Direction, usefd bool, endpoint multiaddr.Multiaddr) (network.ConnManagementScope, error) {
// Temporary workaround to provide backwards compatibility for users that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the correct behavior and we should remove the temporary caveat.

We shouldn't check for circuit-v2 either. Someone can use their custom transport with tor / bluetooth etc and would prefer to still use the total connection limiting of Open Connection.

If someone is implementing their own resource manager it's that implementations responsibility to the connection limiting based on endpoint. In our case if someone wants to limit their circuit v2 connections they can embed this resource manager, override OpenConnection, check if the transport is a circuit-v2 transport and then do their custom limiting for it otherwise call the underlying rcmgr.OpenConneciton

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reason for having OpenConnectionNoIP is to make it an explicit decision by the caller to not pass IP information in the multiaddr. If transport that uses IP (all but circuitv2 right now) is improperly configured, then it should fail when calling OpenConnection without an IP address.

That said, you could argue that's the fault of the transport, and the ResourceManager shouldn't try to protect against bad transport implementations. Especially since this is just one way they could fail. And you could argue that the OpenConnection implementation using IP information is just what the default concrete resource manager does, but may not be something another resource manager implementation does (which I think is your point). That's a fair point too.

I think you're right. The counter points seem stronger than the reason for having them. Even if I prefer the explicit decision by the caller, it does change the interface to be IP aware, which was not the goal with the original interface. I'll update this PR and close #2806

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially since this is just one way they could fail. And you could argue that the OpenConnection implementation using IP information is just what the default concrete resource manager does, but may not be something another resource manager implementation does (which I think is your point)

Exactly, why single out the property that it's an IP address or a non IP address?

@MarcoPolo MarcoPolo requested a review from sukunrt May 22, 2024 18:53
p2p/host/resource-manager/rcmgr.go Outdated Show resolved Hide resolved
p2p/host/resource-manager/rcmgr.go Outdated Show resolved Hide resolved
@sukunrt sukunrt merged commit 62ebf06 into master May 23, 2024
10 of 11 checks passed
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

Successfully merging this pull request may close these issues.

2 participants