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

Idiomatic default CompressionMode #230

Closed
tayadelmar opened this issue Apr 29, 2020 · 6 comments
Closed

Idiomatic default CompressionMode #230

tayadelmar opened this issue Apr 29, 2020 · 6 comments
Milestone

Comments

@tayadelmar
Copy link

It would be idiomatic to have CompressionDisabled as a default CompressionMode.

Now it's a bit confusing, because the default mode is CompressionNoContextTakeover and the lib encourages to use context.Context, which is actually very cool, But using the default compression mode with the context leads to a reading error:

failed to read JSON message: failed to read: flate: corrupt input before offset 11

So user needs to investigate the error and research about compression modes if he wants just to connect with default DialOptions.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 30, 2020

Hmm that's an unexpected error. What client are you getting that from? Indicates a bug somewhere.

But either way, compression is going to be changed to be disabled by default. See #220

@tayadelmar
Copy link
Author

The client is:

type Ws struct {
	conn *websocket.Conn
}

func Connect(ctx context.Context) (*Ws, error) {
	c, _, err := websocket.Dial(ctx, "wss://testnet.bitmex.com/realtime", nil)
	if err != nil {
		return nil, fmt.Errorf("dial failed: %w", err)
	}

	ws:=&Ws{c}

	go ws.readLoop()

	return ws, nil
}

func (ws *Ws) readLoop() {
	for {
		var msg interface{}

		ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
		defer cancel()

		if err := wsjson.Read(ctx, ws.conn, &msg); err != nil {
			log.Println(err)
			break
		}

		log.Println("message: ", msg)
	}
}

func (ws *Ws) WriteJSON(ctx context.Context, v interface{}) {
	ctx, cancel := context.WithTimeout(ctx, 15*time.Second)
	defer cancel()
	return wsjson.Write(ctx, ws.conn, v)
}

func (ws *Ws) Disconnect() {
	ws.conn.Close(websocket.StatusNormalClosure, "")
}

I try to subscribe to a trade feed, for example:

func TestSubscription(t *testing.T) {
	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
	defer cancel()

	ws, _ := Connect(ctx)
	defer ws.Disconnect()

	msg := map[string]interface{}{
		"op":   "subscribe",
		"args": []string{"trade:XRPM20"},
	}

	ctx, cancel = context.WithTimeout(context.Background(), 3*time.Second)
	defer cancel()

	_ = ws.WriteJSON(ctx, msg)

	// wait till message received
}

After connect I get an info message:

{"docs":"https://testnet.bitmex.com/app/wsAPI","info":"Welcome to the BitMEX Realtime API.","limit":{"remaining":39},"timestamp":"2020-04-26T19:17:24.517Z","version":"2020-04-20T21:56:03.000Z"}

Then I write message to subscribe and get the reading error:

failed to read JSON message: failed to read: flate: corrupt input before offset 11

The message, which failed to read, is:

{"request":{"args":["trade:XRPM20"],"op":"subscribe"},"subscribe":"trade:XRPM20","success":true}

Changing CompressMode to CompressionContextTakeover or CompressionDisabled solves the issue.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 30, 2020

Will debug soon, thanks for reporting!

@nhooyr nhooyr added this to the v1.8.7 milestone May 18, 2020
nhooyr added a commit that referenced this issue May 18, 2020
nhooyr added a commit that referenced this issue May 18, 2020
nhooyr added a commit that referenced this issue May 18, 2020
@nhooyr
Copy link
Contributor

nhooyr commented May 18, 2020

@tayadelmar What happens if you use CompressionContextTakeover?

@tayadelmar
Copy link
Author

CompressionContextTakeover doesn't cause the error, which I mentioned above.

@nhooyr
Copy link
Contributor

nhooyr commented May 18, 2020

Awesome, likely looks like it's a bug in their service then that doesn't negotiate context takeover correctly as we test against autobahn but I'll double check the code.

nhooyr added a commit that referenced this issue May 18, 2020
@nhooyr nhooyr closed this as completed Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants