Philipp15b / go-steam

Steam's protocol in Go to allow automation of different actions on the Steam network without running an actual Steam client. Includes APIs for friends, chatting, trading, trade offers and TF2 crafting.
https://pkg.go.dev/github.com/Philipp15b/go-steam/v3
Other
387 stars 131 forks source link

Context aware Connect() functions #136

Open Delicious-Bacon opened 12 months ago

Delicious-Bacon commented 12 months ago

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 commented 12 months ago

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
    }