go-socket.io icon indicating copy to clipboard operation
go-socket.io copied to clipboard

Concurrent encoding/decoding

Open eyudkin opened this issue 7 years ago • 10 comments

I found, that mutex (passing to encoder/decoder) does not work, because it should be passed by reference. For now it looks like:

func NewDecoder(r FrameReader, mtx sync.Mutex) *Decoder {
	return &Decoder{
		r: r,
		mtx: mtx,
	}
}

(should be NewDecoder(r FrameReader, mtx *sync.Mutex) ) And at the result, decoder and encoder have different mutexes and it causes bad situations when application tries to emit something in the same moment with the PING request and etc.

Unfortunately, I can not fix it fastly, because just making references to one mutex causes deadlock: DecodeHeader method locks mutex and causes infinite NextReader() cycle and so mutex will not be free until any non-ping message from client (so, it blocks server->client messages).

eyudkin avatar Jun 16 '18 12:06 eyudkin

Which branch? v1.4 or master?

googollee avatar Jun 16 '18 13:06 googollee

1.4

eyudkin avatar Jun 16 '18 13:06 eyudkin

You mean https://github.com/googollee/go-socket.io/blob/v1.4/parser/decoder.go#L29 ?

googollee avatar Jun 16 '18 13:06 googollee

Oops, sorry, I forgot that I made some changes in my fork. This mutex was my fix for some concurrent situations.

eyudkin avatar Jun 16 '18 13:06 eyudkin

One of them - its writePacket call in encoder.Encode function.

func (e *Encoder) Encode(h Header, args []interface{}) (err error) {
	var w io.WriteCloser
	w, err = e.w.NextWriter(engineio.TEXT)
	if err != nil {
		return
	}

	var buffers [][]byte
	buffers, err = e.writePacket(w, h, args)

	if err != nil {
		return
	}

	for _, b := range buffers {
		w, err = e.w.NextWriter(engineio.BINARY)
		if err != nil {
			return
		}
		err = e.writeBuffer(w, b)
		if err != nil {
			return
		}
	}
	return
}

So, calls like e.w.NextWriter(engineio.TEXT) works with session (it has upgradeLocker mutex) - and its safe. But writePacket - no, and (as I understood) in some cases it closes writer, when reader works with it.

eyudkin avatar Jun 16 '18 13:06 eyudkin

So, @googollee - issue is still actual (as for me) ;)

eyudkin avatar Jun 16 '18 13:06 eyudkin

Encoder.writePacket() should not race with eath other. NextWriter() will block each other and only one return writer to do writePacket(). Could you give an example to show the issue?

googollee avatar Jun 17 '18 03:06 googollee

Code screenshot Stacktraces screenshot As I understand the situation: serveWrite->encode->writePacket closes connection without any mutexes (WriteCloser interface), but serveRead ->DecodeHeader->... still works with it in other goroutine. So, my error text is: websocket: write closed

eyudkin avatar Jun 18 '18 09:06 eyudkin

@eyudkin Hi Is it actual problem?

sshaplygin avatar Oct 22 '19 14:10 sshaplygin

I think we have same issue. We will try to make a test to reproduce it. It seems that appears when we broadcast to many events in short delay with at least one subscriber

julienmathevet avatar Feb 06 '20 11:02 julienmathevet