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

implement connection gating at the top level #881

Merged
merged 8 commits into from
May 15, 2020
Merged

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Apr 9, 2020

For #872.

  • This PR deprecates the public Filter and FilterAddress API in favour of the ConnectionGater API.
  • Users can use either a Filter and by extension FilterAddress OR the ConnectionGater but NOT both.
  • Filters is adapted to a ConnectionGater by means of FilterConnectionGater
  • Adds the ConenctionGater to the "reflection magic" so it is available to Transports that need it.
    • While transports that need upgrading like TCP & WS will have their job done by the Upgrader, transports like QUIC (which do not need upgrading) should use this injected gater to gate connections.

@raulk raulk changed the title Deprecate Filters with ConnectionGater and inject it to Swarm and Upgrader implement connection gating at the top level May 15, 2020
@raulk raulk merged commit 887f2c4 into master May 15, 2020
@raulk raulk deleted the feat/connection-gating branch May 15, 2020 16:31
@@ -76,7 +78,7 @@ func callConstructor(c reflect.Value, args []reflect.Value) (interface{}, error)
return val, err
}

type constructor func(h host.Host, u *tptu.Upgrader) interface{}
type constructor func(h host.Host, u *tptu.Upgrader, cg connmgr.ConnectionGater) interface{}
Copy link
Member

Choose a reason for hiding this comment

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

@aarshkshah1992

What was the motivation to change this function signature? It's not really an issue, but it's redundant with the ConnGater field of the upgrader.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I had the same thought but given the types are internal, I thought it wouldn't harm us to rectify later. Wanna tackle that as a follow-up, @aarshkshah1992?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien @raulk

I think this was an oversight. I'll create a follow up PR to get rid of this. #951 .

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Note: this isn't a super high priority.

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.

3 participants