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

wsjson.Write write in a single frame always #315

Closed
dar7man opened this issue Aug 21, 2021 · 15 comments
Closed

wsjson.Write write in a single frame always #315

dar7man opened this issue Aug 21, 2021 · 15 comments
Milestone

Comments

@dar7man
Copy link

dar7man commented Aug 21, 2021

Hi,

I have a weird problem using this package with Binance WebSocket API. Just after I got stream subscription confirmation, connection is closed (code 1008). At the beginning I was thinking about contacting with Binance support, but then I tried the same code but using Gorilla WebSockets package and... it worked fine.

Here is code nhooyr package version (not working):

package main

import (
	"context"
	"log"
	"os"
	"os/signal"
	"time"

	"nhooyr.io/websocket"
	"nhooyr.io/websocket/wsjson"
)

type subscribeMessage struct {
	Method string   `json:"method"`
	Params []string `json:"params"`
	Id     int      `json:"id"`
}

func main() {
	interrupt := make(chan os.Signal, 1)
	signal.Notify(interrupt, os.Interrupt)
	ctx, cancel := context.WithTimeout(context.Background(), time.Second*30)
	defer cancel()

	c, _, err := websocket.Dial(ctx, "wss://stream.binance.com:9443/stream", nil)
	if err != nil {
		log.Println("dial:", err)
		return
	}
	defer func() { _ = c.Close(websocket.StatusNormalClosure, "") }()

	if err = wsjson.Write(ctx, c, subscribeMessage{
		Method: "SUBSCRIBE",
		Params: []string{"wavesbtc@depth@100ms"},
		Id:     100,
	}); err != nil {
		log.Println("write:", err)
		return
	}

	done := make(chan struct{})

	go func() {
		defer close(done)
		for {
			_, message, err := c.Read(ctx)
			if err != nil {
				log.Println("read:", err)
				return
			}
			log.Printf("recv: %s", message)
		}
	}()

	select {
	case <-interrupt:
		log.Println("interrupt")
	case <-done:
		return
	}

	err = c.Close(websocket.StatusNormalClosure, "")
	if err != nil {
		log.Println("write close:", err)
		return
	}
	<-done
}

And here is Gorilla version:

package main

import (
	"context"
	"log"
	"os"
	"os/signal"
	"time"

	"github.com/gorilla/websocket"
)

type subscribeMessage struct {
	Method string   `json:"method"`
	Params []string `json:"params"`
	Id     int      `json:"id"`
}

func main() {
	interrupt := make(chan os.Signal, 1)
	signal.Notify(interrupt, os.Interrupt)
	ctx, cancel := context.WithTimeout(context.Background(), time.Second*30)
	defer cancel()

	c, _, err := websocket.DefaultDialer.DialContext(ctx, "wss://stream.binance.com:9443/stream", nil)
	if err != nil {
		log.Println("dial:", err)
		return
	}
	defer func() { _ = c.Close() }()
	err = c.WriteJSON(subscribeMessage{
		Method: "SUBSCRIBE",
		Params: []string{"wavesbtc@depth@100ms"},
		Id:     100,
	})
	if err != nil {
		log.Println("write:", err)
		return
	}
	done := make(chan struct{})

	go func() {
		defer close(done)
		for {
			_, message, err := c.ReadMessage()
			if err != nil {
				log.Println("read:", err)
				return
			}
			log.Printf("recv: %s", message)
		}
	}()

	select {
	case <-interrupt:
		log.Println("interrupt")
	case <-done:
		return
	}

	err = c.WriteMessage(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseNormalClosure, ""))
	if err != nil {
		log.Println("write close:", err)
		return
	}
	<-done
	return
}

Any ideas what's wrong?
Thanks!

@nhooyr
Copy link
Contributor

nhooyr commented Aug 21, 2021

Hi @dar7man

Yea super weird looks perfect to me. Can you try the dev branch as well? Just go get nhooyr.io/websocket@dev

@dar7man
Copy link
Author

dar7man commented Aug 22, 2021

Hi @nhooyr

Thanks for quick response. I tried dev brach, but not luck. Still doesn't work. Scenario is the same:

  1. Connection success
  2. Write: success (to subscribe to given channel)
  3. First Read in loop: success (response with confirmation of subscription, no errors here)
  4. Second Read in loop: error: read: failed to get reader: received close frame: status = StatusPolicyViolation and reason = "Invalid request"

Super weird. Reason is "Invalid request" but request is valid, so I'm guessing this is just some general error.

@nhooyr
Copy link
Contributor

nhooyr commented Aug 23, 2021

Huh, yea that's really bizarre.

Only difference between I see is the Id used in this library's demo is 100 but for Gorilla it's 1. Could that be the reason?

@dar7man
Copy link
Author

dar7man commented Aug 23, 2021

No, unfortunately. This number doesn't matter. Looks like something different must be sent using your package. I don't know, maybe packet sniffing/monitoring could help with investigation? I now that Windows 10 had build in such a tool (pktmon.exe). Maybe I'll try some day to use it and try to find a difference.

@dar7man
Copy link
Author

dar7man commented Aug 25, 2021

Got some news here. Looks like getting rid of wsjson.Write() solves the problem.
After replacing it with json marshaling and plain Write() it works:

msg, err := json.Marshal(subscribeMessage{
  Method: "SUBSCRIBE",
  Params: []string{"wavesbtc@depth@100ms"},
  Id:     100,
})
if err != nil {
  log.Println("write:", err)
  return
}
if err = c.Write(ctx, websocket.MessageText, msg); err != nil {
  log.Println("write:", err)
  return
}

I was suspecting that json Encoder my be doing something that Binance don't like, so I compared output for the same request from Marshal and from Encoder. Indeed there is a difference: Encoder is adding extra line feed and the end of buffer (ASCII: 10).

So I send buffer prepared by Encoder to Binance and... it still works (this extra line feed and the end doesn't make a difference).
Very strange... Only difference I see is that I'm using plain Write() method but wsjson.Write() is using Writer... Any ideas what my be wrong?

I'll try later use Writer, not Write and see what happens.

PS. Oh! One more thing to mention: this workaround only works with dev branch!

@XanderShum
Copy link

I ran into the same problem, thank you for your help.

@zeroallox
Copy link

I'm experiencing this same issue while using normal Write(). Have not even tried wsjson.Write() or any of the util methods. Using main branch / latest release.

@elee1766
Copy link

elee1766 commented Nov 11, 2022

even if its not the issue with binance api, the newline thing is an issue i have had with other apis, and the best way i found to fix it was just to wrap the writer, since encoding/json writes the entire encoded json buffer to the underlying writer at once.

eg:

import "io"

// usage: json.NewEncoder(&noTrailingNewlineWriter{w})
type noTrailingNewlineWriter struct {
  w io.Writer
}

func (n *noTrailingNewlineWriter) Write(xs []byte) (int, error) {
  if len(xs) == 0 {
    return 0, nil
  }
  if xs[len(xs)-1] == '\n' {
    xs = xs[:len(xs)-1]
  }
  return n.w.Write(xs)
}

i actually use this in one of my projects since it was an issue with a project i was doing, i just copied wsjson into that project. i copied it out here if anyone needs it

https://github.com/elee1766/websocket/blob/8889fe544bfd9f50638ea2019c9cd183f1a02794/wsjson/wsjson.go#L61

@nhooyr
Copy link
Contributor

nhooyr commented Mar 5, 2023

Yea so binance doesn't support fragmented websocket frames for some reason. This library normally writes fragmented frames but the dev branch fixes it to not with Write. So after dev is merged into master I'll close this.

@timo-klarshift
Copy link

can it be closed?

@dar7man
Copy link
Author

dar7man commented May 4, 2023 via email

@nhooyr nhooyr added this to the v1.8.8 milestone Sep 28, 2023
@nhooyr
Copy link
Contributor

nhooyr commented Oct 13, 2023

Actually I took another look at the code and v1.8.7 includes the fix to change Write to guarantee writing a single frame but only if compression is disabled.

So the fix here is don't use wsjson as that will not write the result in a single frame but also disable compression with v1.8.7.

v1.8.8 will disable compression by default so it won't be an issue in the next release.

Sorry for the confusion.

@nhooyr nhooyr closed this as completed Oct 13, 2023
@nhooyr
Copy link
Contributor

nhooyr commented Oct 13, 2023

Actually in v1.8.8 I'll change wsjson.Write to just make it work. The reason it doesn't is because I was trying to be clever and use json.Encoder instead of json.Marshal as encoder reuses the buffer it writes the JSON too whereas json.Marshal always does an allocation.

@nhooyr nhooyr reopened this Oct 13, 2023
@nhooyr
Copy link
Contributor

nhooyr commented Oct 13, 2023

But I can still use json.Encoder and have it call Write on the connection instead of Writer. So this will be fixed next release.

@nhooyr nhooyr changed the title Not working with Binance WebSocket API wsjson.Write write in a single frame always Oct 13, 2023
nhooyr added a commit that referenced this issue Oct 13, 2023
@nhooyr
Copy link
Contributor

nhooyr commented Oct 13, 2023

Done in dev.

@nhooyr nhooyr closed this as completed Oct 13, 2023
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

6 participants