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.

Sign up for GitHub

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

Jump to bottom

when the connection is broken, join() without Close() freezes for ten… #137

Closed
wants to merge 2 commits into from
Closed

when the connection is broken, join() without Close() freezes for ten… #137

wants to merge 2 commits into from

Conversation

abakum
Copy link
Contributor

@abakum abakum commented Nov 5, 2023

…s of seconds

@abakum
Copy link
Contributor Author

abakum commented Nov 7, 2023 

func join(ctx context.Context, left, right net.Conn) {
	g, _ := errgroup.WithContext(ctx) // when ctx is canceled (on WithStopHandler or WithDisconnectHandler ) interrupts both io.Copy 
	g.Go(func() error {
		_, err := io.Copy(left, right)
		left.Close() // on left disconnection interrupts io.Copy(right, left) 
		return err
	})
	g.Go(func() error {
		_, err := io.Copy(right, left)
		right.Close() // on right disconnection interrupts io.Copy(left, right) 
		return err
	})
	g.Wait()

}

@Haythmaltbey Haythmaltbey mentioned this pull request Jan 26, 2024
Open
euank pushed a commit to euank/libngrok-go that referenced this pull request May 28, 2024
@abakum @euank
Propagate half-closes correctly in forward
981f6b2 
Before, the following would not work as you would expect:

```go
// # One terminal
// $ ncat --recv-only -l 9090

// ngrok-go code
fwd, err := sess.ListenAndForward(
  ctx,
  "127.0.0.1:9090",
  config.TCPEndpoint(),
)

// fwd.URL() is 0.tcp.jp.ngrok.io:14517 for this example

// another terminal
// $ ncat --send-only 0.tcp.jp.ngrok.io 14517 < hello-world.txt
```

What we would expect from the above would be for the send side to send
"hello world" and exit, and then the recv side to print "hello world"
and also exit.

This is what happens if you do `ncat --send-only localhost 9090`
instead of copying through the ngrok tcp tunnel.

Before this change, when copying through ngrok the recv side would not
exit because the 'Close' of the connection did not get propagated
through the 'join'.

I've also added a unit test showing this.

Thank you to @abakum for originally noticing this issue and offering a
fix over in ngrok#137.
In the hopes of landing this more quickly, I've written a new version, derived from
the internal ngrok agent's join code, which should thus be easier to
review etc.
@euank euank mentioned this pull request May 28, 2024
Merged
euank pushed a commit to euank/libngrok-go that referenced this pull request May 28, 2024
@abakum @euank
Propagate half-closes correctly in forward
f64ed57 
Before, the following would not work as you would expect:

```go
// # One terminal
// $ ncat --recv-only -l 9090

// ngrok-go code
fwd, err := sess.ListenAndForward(
  ctx,
  "127.0.0.1:9090",
  config.TCPEndpoint(),
)

// fwd.URL() is 0.tcp.jp.ngrok.io:14517 for this example

// another terminal
// $ ncat --send-only 0.tcp.jp.ngrok.io 14517 < hello-world.txt
```

What we would expect from the above would be for the send side to send
"hello world" and exit, and then the recv side to print "hello world"
and also exit.

This is what happens if you do `ncat --send-only localhost 9090`
instead of copying through the ngrok tcp tunnel.

Before this change, when copying through ngrok the recv side would not
exit because the 'Close' of the connection did not get propagated
through the 'join'.

I've also added a unit test showing this.

Thank you to @abakum for originally noticing this issue and offering a
fix over in ngrok#137.
In the hopes of landing this more quickly, I've written a new version, derived from
the internal ngrok agent's join code, which should thus be easier to
review etc.

To try and give credit correctly, I've maintained the original commits
from ngrok#137 as well.
euank added a commit to euank/libngrok-go that referenced this pull request May 28, 2024
@euank
Propagate half-closes correctly in forward
c67a6d2 
Before, the following would not work as you would expect:

```go
// # One terminal
// $ ncat --recv-only -l 9090

// ngrok-go code
fwd, err := sess.ListenAndForward(
  ctx,
  "127.0.0.1:9090",
  config.TCPEndpoint(),
)

// fwd.URL() is 0.tcp.jp.ngrok.io:14517 for this example

// another terminal
// $ ncat --send-only 0.tcp.jp.ngrok.io 14517 < hello-world.txt
```

What we would expect from the above would be for the send side to send
"hello world" and exit, and then the recv side to print "hello world"
and also exit.

This is what happens if you do `ncat --send-only localhost 9090`
instead of copying through the ngrok tcp tunnel.

Before this change, when copying through ngrok the recv side would not
exit because the 'Close' of the connection did not get propagated
through the 'join'.

I've also added a unit test showing this.

Thank you to @abakum for originally noticing this issue and offering a
fix over in ngrok#137.
In the hopes of landing this more quickly, I've written a new version, derived from
the internal ngrok agent's join code, which should thus be easier to
review etc.

To try and give credit correctly, I've maintained the original commits
from ngrok#137 as well.
euank added a commit that referenced this pull request May 29, 2024
@euank
Propagate half-closes correctly in forward
27de5d7 
Before, the following would not work as you would expect:

```go
// # One terminal
// $ ncat --recv-only -l 9090

// ngrok-go code
fwd, err := sess.ListenAndForward(
  ctx,
  "127.0.0.1:9090",
  config.TCPEndpoint(),
)

// fwd.URL() is 0.tcp.jp.ngrok.io:14517 for this example

// another terminal
// $ ncat --send-only 0.tcp.jp.ngrok.io 14517 < hello-world.txt
```

What we would expect from the above would be for the send side to send
"hello world" and exit, and then the recv side to print "hello world"
and also exit.

This is what happens if you do `ncat --send-only localhost 9090`
instead of copying through the ngrok tcp tunnel.

Before this change, when copying through ngrok the recv side would not
exit because the 'Close' of the connection did not get propagated
through the 'join'.

I've also added a unit test showing this.

Thank you to @abakum for originally noticing this issue and offering a
fix over in #137.
In the hopes of landing this more quickly, I've written a new version, derived from
the internal ngrok agent's join code, which should thus be easier to
review etc.

To try and give credit correctly, I've maintained the original commits
from #137 as well.
@euank
Copy link
Contributor

euank commented May 29, 2024

I carried forward and merged an equivalent change over in #163, closing this one as resolved via that PR.
Thanks!

@euank euank closed this May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@abakum @euank