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

Context aware Connect() functions #136

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Delicious-Bacon
Copy link

@Delicious-Bacon Delicious-Bacon commented Sep 10, 2023

Issue

InitializeSteamDirectory() returns the list of Steam servers, but some of them are bad and hang until the OS decides to close the connection. This causes issues when using the client.Connect() method that decides to pick up a "bad" server:

// with 162.254.198.104:27018 being the "bad" server.
ConnectTo failed: dial tcp 162.254.198.104:27018: connect: connection timed out 

This also causes the client to call Fatalf() method and emit Fatal event:

conn, err := dialTCP(local, addr.ToTCPAddr())
if err != nil {
	c.Fatalf("Connect failed: %v", err)
	return err
}

Not sure why, but upon calling the client.Connect() the next time, it either hangs forever, or manages to connect to a new server (unless it picks up a bad server again), but then panics when trying to login (client.Auth.LogOn()).


This is probably the func that panics with negative index slice slicing (something like s[:-221]):

func unpadPKCS7(src []byte) []byte {
	padLen := src[len(src)-1]
	return src[:len(src)-int(padLen)]
}

Proposal

Proposed addition is to add Context aware functions which would let users pass the context with timeout on dial.

+ ConnectContext()
+ ConnectToContext()
+ ConnectToBindContext()
+ dialTCPContext()

It is up to the user on how to handle the context.Canceled or context.DeadlineExceeded error.

ConnectToBindContext() would also not emit Fatalf() event error on context cancellation:

conn, err := dialTCPContext(ctx, local, addr.ToTCPAddr())
if err != nil {
	if !(errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)) {
		c.Fatalf("Connect failed: %v", err)
	}
	return err
}

Context aware functions are tested in the newly added client_test.go.

Note

One minor thing to note is that the implementation would use a workaround on net.DialTCP() because net.DialTCPContext() is accepted, but awaiting implementation in Go (https://go-review.googlesource.com/c/go/+/490975). The only thing this does is that we leave the coroutine waiting for the dial to connect, but the connection, whether connected or not after context times out, is discarded.

func dialTCPContext(ctx context.Context, laddr, raddr *net.TCPAddr) (*tcpConnection, error) {

	// TODO add DialTCPContext once it's available.
	// Currently a workaround.
	ok := make(chan struct{})
	var (
		conn *net.TCPConn
		err  error
	)
	go func() { // This coroutine would run until hang.
		conn, err = net.DialTCP("tcp", laddr, raddr)
		close(ok)
	}()

	select {
	case <-ctx.Done():
		return nil, ctx.Err() // Discards the connection.
	case <-ok:
	}

@Delicious-Bacon
Copy link
Author

In force-pushed amend bece4e813e474b842cb4246af0bbaf216ea89468:

Fixed a bug:

select {
case <-ctx.Done():
-       close(ok)
	return nil, ctx.Err()
case <-ok:
}

Added context.DeadlineExceeded check:

func (c *Client) ConnectToBindContext(ctx context.Context, addr *netutil.PortAddr, local *net.TCPAddr) error {
	c.Disconnect()

	conn, err := dialTCPContext(ctx, local, addr.ToTCPAddr())
	if err != nil {
-		if !errors.Is(err, context.Canceled) {
+		if !(errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)) {
			c.Fatalf("Connect failed: %v", err)
		}
		return err
	}

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.

1 participant