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

tcpreuse: error on using tcpreuse with pnet #3129

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Dec 30, 2024

Sharing the TCP Listen socket with WebSocket doesn't work with pnet.

Sharing the TCP Listen socket with WebSocket doesn't work with pnet.
@sukunrt sukunrt requested a review from aschmahmann December 30, 2024 15:47
Copy link
Collaborator

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Left some questions/comments, but generally LGTM

config/config.go Outdated
Comment on lines 484 to 486
if len(cfg.PSK) > 0 && cfg.ShareTCPListener {
return nil, errors.New("cannot use shared TCP and WebSocket listener with PSK")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM, although technically the shared listener feature isn't coupled to having TCP + WS since you can currently enable it with just TCP (albeit at no real benefit) and down the road it might be configurable with HTTP as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing this error test to just say "... shared TCP listener ...". This is in line with option one in #3119.

config/config.go Outdated
Comment on lines 454 to 455
defer func() {
if createErr != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this cover everything not already stopped by fx and how does this play with the closing that happens when autonat fails to start?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This instantiation is so hacky :(. I believe if autonat fails this will end up closing things twice.

Maybe slightly better would be to move the config validation logic to a separate fn. Then cleanup on if that fn returns an error.

@p-shahi p-shahi linked an issue Dec 30, 2024 that may be closed by this pull request
@MarcoPolo MarcoPolo self-requested a review January 9, 2025 22:04
@MarcoPolo MarcoPolo merged commit ca611f1 into master Jan 10, 2025
11 checks passed
sukunrt added a commit that referenced this pull request Jan 21, 2025
2color added a commit that referenced this pull request Jan 24, 2025
…bug-nat-port-mapping

* origin/marco/bring-go-nat-home: (75 commits)
  go mod tidy
  reference internal package for mockgen
  mod tidy test-plans
  Make changes to internal nat library
  ci: move to actions/upload-artifact@v4 (#3152)
  tcpreuse: fix rcmgr accounting when tcp metrics are enabled (#3142)
  fix(net/nat): data race problem of `extAddr` (#3140)
  test: fix failing test (#3141)
  quicreuse: make it possible to use an application-constructed quic.Transport (#3122)
  nat: ignore mapping if external port is 0 (#3094)
  tcpreuse: error on using tcpreuse with pnet (#3129)
  chore: Update contribution guidelines (#3134)
  tcp: fix metrics test build directive (#3052)
  webrtc: upgrade pion/webrtc to v4 (#3098)
  ci: get back on the main release track of release checker (#3117)
  webtransport: fix docstring comment for getCurrentBucketStartTime
  chore: release v0.38.1 (#3114)
  fix(httpauth): Correctly handle concurrent requests on server (#3111)
  ci: Install specific protoc version when generating protobufs (#3112)
  fix(autorelay): Move relayFinder peer disconnect cleanup to separate goroutine (#3105)
  ...
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.

TCP connection multiplexing (tcpreuse package) doesn't work with pnet
3 participants