Skip to content

Add StartContext to ICETransport#3371

Open
HashimTheArab wants to merge 2 commits intopion:masterfrom
HashimTheArab:add-ice-transport-start-context
Open

Add StartContext to ICETransport#3371
HashimTheArab wants to merge 2 commits intopion:masterfrom
HashimTheArab:add-ice-transport-start-context

Conversation

@HashimTheArab
Copy link

@HashimTheArab HashimTheArab commented Feb 10, 2026

Summary

  • Adds StartContext method to ICETransport that accepts a context.Context parameter, allowing callers to control cancellation and timeouts for ICE connectivity checks
  • Refactors Start to delegate to StartContext with context.Background(), maintaining full backwards compatibility

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Sean-Der
Copy link
Member

Seems useful @HashimTheArab! I’m curious what’s the bug/use case your solving for?

@codecov
Copy link

codecov bot commented Feb 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.98%. Comparing base (374c864) to head (d95348d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3371      +/-   ##
==========================================
+ Coverage   84.97%   84.98%   +0.01%     
==========================================
  Files          81       81              
  Lines        9637     9639       +2     
==========================================
+ Hits         8189     8192       +3     
+ Misses       1022     1020       -2     
- Partials      426      427       +1     
Flag Coverage Δ
go 84.98% <100.00%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@RealSexMC
Copy link

RealSexMC commented Feb 10, 2026

We're using this for Minecraft Nethernet connections (https://github.com/lactyy/go-nethernet) and the transport methods can hang indefinitely if the remote peer never responds. Without this option we have to use this fragile hack:

func withContextCancel(ctx context.Context, f func() error, cancel func()) error {
	errCh := make(chan error, 1)
	go func() {
		errCh <- f()
	}()
	select {
	case <-ctx.Done():
		if cancel != nil {
			cancel()
		}
		go func() { <-errCh }()
		return ctx.Err()
	case err := <-errCh:
		return err
	}
}

Passing a context will allow us to simplify and improve all of the call sites:

  // Before
  if err := withContextCancel(ctx, func() error {
      return conn.ice.Start(nil, d.ice, &iceRole)
  }, func() {
      _ = conn.ice.Stop()
  }); err != nil {
      return fmt.Errorf("start ICE: %w", err)
  }

  // After
  if err := conn.ice.StartContext(ctx, nil, d.ice, &iceRole); err != nil {
      return fmt.Errorf("start ICE: %w", err)
  }

(I am not the author of the PR)

@JoTurk
Copy link
Member

JoTurk commented Feb 10, 2026

@RealSexMC this is interesting I'm curious why you don't use pion/ice directly? seems more fit for your use, but I might be missing something. We're currently working on improving pion/ice and making it more suitable for non-webrtc use.

@didntpot
Copy link

@RealSexMC that is very interesting, couldn't you

@HashimTheArab
Copy link
Author

NetherNet is a full WebRTC stack, it uses ICE + DTLS + SCTP with data channels (ReliableDataChannel and UnreliableDataChannel) negotiated via SDP. We use pion's ORTC API to set up each transport individually since the signaling is custom (not standard WebSocket/HTTP), but the transports themselves are standard WebRTC.

@HashimTheArab
Copy link
Author

Anything else required to get this merged?

@JoTurk
Copy link
Member

JoTurk commented Feb 15, 2026

Can you please fix the lint?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants