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

feat: stop all active connections on Proxy.Stop() #206

Merged
merged 7 commits into from
Apr 2, 2024
Merged

Conversation

fortuna
Copy link
Contributor

@fortuna fortuna commented Mar 26, 2024

This is to ensure no connection lingers around. I'm hoping this will fix the issue on iOS. With this change, Shutdown always returns quickly.

I also realized that the Hijacker returns a bufio.readWriter, so we might as well leverage the buffer that the http server already allocated in the io.Copy. It implements ReadFrom and WriteTo.

@fortuna fortuna requested a review from jyyi1 March 26, 2024 03:08
Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Apart from this PR, shall we introduce a Close or Shutdown function to every StreamDialer, I feel it's a common feature for a StreamDialer to stop the connections?

x/mobileproxy/stop_dialer.go Outdated Show resolved Hide resolved
x/mobileproxy/mobileproxy.go Show resolved Hide resolved
x/mobileproxy/stop_dialer.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

Comments addressed. It turns out it's more complicated than I thought.

I'm now handling the case where stop happens before or during DialStream.

I added tests to ensure I'm covering all the cases.

x/mobileproxy/stop_dialer.go Outdated Show resolved Hide resolved
x/mobileproxy/mobileproxy.go Show resolved Hide resolved
x/mobileproxy/stop_dialer.go Outdated Show resolved Hide resolved
@fortuna fortuna requested a review from jyyi1 March 29, 2024 22:39
Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

Logic looks good, but please double check the package comment.

x/mobileproxy/stop_dialer.go Outdated Show resolved Hide resolved
Comment on lines 15 to 18
// Package mobileproxy provides convenience utilities to help applications run a local proxy
// and use that to configure their networking libraries.
//
// This package is suitable for use with Go Mobile, making it a convenient way to integrate with mobile apps.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Package mobileproxy provides convenience utilities to help applications run a local proxy
// and use that to configure their networking libraries.
//
// This package is suitable for use with Go Mobile, making it a convenient way to integrate with mobile apps.

Copy link
Contributor Author

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

@jyyi1 Please take another look.

I rethought this PR, and it's much simpler now. Instead of driving the cancelation via the dialer, it's much easier if we drive the cancelation via the client connection. We use a base context to notify the handlers the request is cancelled.

We can use context.AfterFunc like you suggested, but we need to upgrade to 1.21 first.

@fortuna fortuna requested a review from jyyi1 April 2, 2024 00:19
Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

Thanks, that's a lot simpler now!

@fortuna fortuna merged commit 49ebf7c into main Apr 2, 2024
6 checks passed
@fortuna fortuna deleted the fortuna-stop branch April 2, 2024 21:57
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.

None yet

2 participants