Proposal: New candidate gathering API

[stv0g]

This is an initial proposal for harmonizing the candidate gathering process. Specifically, it attempts to provide the users a possibility to implement custom logic for gathering candidates.

I aim to improve the state of UDPMuxes with the PR in the long run.

Comments are welcome :)

[codecov[bot]]

Codecov Report

Patch coverage: 2.40% and project coverage change: -2.18 :warning:

Comparison is base (4697f51) 77.65% compared to head (2510201) 75.47%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
- Coverage   77.65%   75.47%   -2.18%     
==========================================
  Files          41       50       +9     
  Lines        4224     4346     +122     
==========================================
  Hits         3280     3280              
- Misses        733      853     +120     
- Partials      211      213       +2     

Flag	Coverage Δ
go	75.47% <2.40%> (-2.18%)	:arrow_down:
wasm	22.04% <0.00%> (-0.66%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
agent.go	81.89% <ø> (ø)
agent_config.go	82.14% <ø> (ø)
gather_static.go	0.00% <0.00%> (ø)
gatherer.go	0.00% <0.00%> (ø)
gatherer_host.go	0.00% <0.00%> (ø)
gatherer_intercept.go	0.00% <0.00%> (ø)
gatherer_intercept_map.go	0.00% <0.00%> (ø)
gatherer_mdns.go	0.00% <0.00%> (ø)
gatherer_multi.go	0.00% <0.00%> (ø)
gatherer_relay.go	0.00% <0.00%> (ø)
... and 2 more

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[stv0g]

I've pushed some more detailed examples how the new gathering API could look like. It is still far from finished. But I would like to receive some feedback before I progress on the wrong path :)

My motivation for the new API:

• Decouple candidate gathering from the rest of the agent logic
  • Simplify testing
  • Allow customization
    • By allowing users to implement the Gatherer interface themself
  • Improve readability, as we separate the code for gathering each type of candidate in its own Gatherer.

My plan going forward:

• For the current version (pion/ice/v2):

  • Add new Gatherer option to AgentConfig
  • If its set, we will use the new gathering API, which will be living in pion/ice alongside the current gathering code (gather.go).
  • If its not set, we will use the old code

• For pion/ice/v3,

  • Remove old gathering code, and make the DefaultGatherer the default
  • Move all the parameters related to candidate gathering to their own data structures
    • Basically move them out of Agent and AgentConfig.
    • Making the agent completely unaware how the candidates have been gathered.

[Sean-Der]

This is fantastic @stv0g very cool.

we can implement stuff like PCP/NAT-PMP. Keep it nice and decoupled. This is gonna be a really fun write up!

It would be nice to do this without API breakage. Why not take the values in AgentConfig and modify these new Gatherers appropriately. If users pass a customer Gatherer then you ignore the old legacy values.

• Big PRs tend to introduce more bugs. It is easier to bisect with small changes.
• As merge conflicts pile up you will get frustrated/burn out
• Others may burn out/get frustrated dealing with large churn as they just want to fix small bugs
• You will (unfortunately) get blamed for issues that aren’t yours because of the uncertainty

It is your call though. I do think you will be happier/more successful if you started small and made these changes in master today!

[stv0g]

Hi @Sean-Der,

It would be nice to do this without API breakage. Why not take the values in AgentConfig and modify these new Gatherers appropriately. If users pass a customer Gatherer then you ignore the old legacy values.

Yes, this is my plan. The ideas outlined for. pion/ice/v3 are just for cleaning up the API. But as you described, we can implement the Gatherer while maintaining API compatibility and gather some experience using the new API, before we fully commit to it.

I am thinking about starting making the Gatherer API fully internal for now with the plan to make it public once I have more confidence.

Regarding your concerns about the big PRs: Luckily, the amount of existing code we need to modify is fairly small:

https://github.com/pion/ice/blob/25102015d4157870fd3a14e35e647c1b60e769f5/gather.go#L59-L63

So, I dont expect a lot of conflicts when rebasing the branch.

[Sean-Der]

Hey @stv0g

I am still in support of this! I think it would be great to move everything into pion/portmap and support lots of other methods of gathering.

For now I am going to close this PR. It isn't actionable and I am working on clearing out the backlog. Tell me if there is anything else I can do to help!