Skip to content

skipping amp transport on large requests#25

Closed
WendelHime wants to merge 5 commits intomainfrom
feat/skip-amp-transport-on-large-request
Closed

skipping amp transport on large requests#25
WendelHime wants to merge 5 commits intomainfrom
feat/skip-amp-transport-on-large-request

Conversation

@WendelHime
Copy link
Contributor

@WendelHime WendelHime commented Dec 16, 2025

AMP requests with payloads larger than 6kb doesn't even reach the broker side

Resolve getlantern/engineering#2871

AMP requests with payloads larger than 6kb doesn't even reach the broker side
@WendelHime WendelHime changed the title feat: skipping amp transport on large requests skipping amp transport on large requests Dec 16, 2025
@WendelHime WendelHime requested a review from Copilot December 16, 2025 18:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds logic to skip the AMP transport when handling HTTP requests with payloads larger than 6KB, addressing an issue where such large requests were failing to reach the broker.

Key Changes:

  • Introduces a 6KB content length limit constant (ampMaxContentLength = 6000)
  • Adds Content-Length header checking logic before launching transport goroutines
  • Skips the AMP transport when request size exceeds the limit

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

name string
}

const ampMaxContentLength = 6000
Copy link
Contributor

@myleshorton myleshorton Dec 16, 2025

Choose a reason for hiding this comment

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

I really think this should be part of a new interface for all transports.

I.e. something like a KindlingTransport interface that would have something like:

MaxLength() int
ConnectedRoundTripper(...) http.RoundTripper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean adding as part of namedRoundTripper function? I'll add a commit that I can revert later if this isn't what you meant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added cb4f44b

this commit might be reverted later if this isn't the correct suggestion implementation
@myleshorton
Copy link
Contributor

myleshorton commented Dec 17, 2025

No I think we should generalize it all like this in kindling.go, with likely just keeping around a slice of transports vs roundTripGenerators.

type Transport interface {
	// NewRoundTripper creates a new http.RoundTripper that is already connected
	NewRoundTripper(ctx context.Context, addr string) (http.RoundTripper, error)
	MaxLength() int
	Name() string
}

func WithTransport(transport Transport) Option {
	return newOption(func(k *kindling) {
		log.Info("Setting custom transport")
		if transport == nil {
			log.Error("Transport instance is nil")
			return
		}
		k.transports = append(k.transports, transport)
	})
}

@myleshorton
Copy link
Contributor

I just started coding this for the example -- should have a PR soon!

@myleshorton
Copy link
Contributor

OK yeah here's that refactor: #26

@WendelHime WendelHime closed this Dec 19, 2025
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