Skip to content

state make more threadsafe + close started chanel#39

Open
tantra35 wants to merge 8 commits intojfreymuth:masterfrom
tantra35:master
Open

state make more threadsafe + close started chanel#39
tantra35 wants to merge 8 commits intojfreymuth:masterfrom
tantra35:master

Conversation

@tantra35
Copy link
Copy Markdown

@tantra35 tantra35 commented Jan 20, 2025

When i begin use this library i found 2 issues:

  1. state in playback not thread safe
  2. started chanel must be closed when Close method of playback is invoked, anotherway we can get deadlock with Start method

@tantra35
Copy link
Copy Markdown
Author

tantra35 commented Jan 20, 2025

Another PR #36

partialy make the same, it only maek state as sync.Atomic, but not close started chanell

@jfreymuth
Copy link
Copy Markdown
Owner

I've merged a different PR for the data race on PlaybackStream.state. I'm not sure I understand what the deadlock is, can you provide an example?

@tantra35
Copy link
Copy Markdown
Author

tantra35 commented Mar 17, 2025

@jfreymuth Ok I wil try to explain:

  1. We neeed to close started chanel https://github.com/jfreymuth/pulse/blob/master/playback.go#L20, in Close method, because this can reduce problems when We call Start simultaneously(we never hung in <-p.started code - https://github.com/jfreymuth/pulse/blob/master/playback.go#L198C3-L198C14) To tell the truth, it won't matter if all operations with the state are done atomically. But for semantic accuracy, it's still worth correcting.

  2. Aproach that you accept is naive. For example Look at Start method, It racy because state doesn't changes atomically, more correct if you doesn't want use atomic, use mutex on whole method Start, or with use of atomic fix it like this:

func (p *PlaybackStream) Start() {
	if p.state.CompareAndSwap(int32(idle), int32(running)) {  // we go here only when succesfuly chnage state from idle to running, and run rest of code only in this case
		p.c.c.Request(&proto.FlushPlaybackStream{StreamIndex: p.index}, nil)
		p.err = nil
		p.request <- int(p.createReply.BufferTargetLength)
		p.underflow = false
		p.c.c.Request(&proto.CorkPlaybackStream{StreamIndex: p.index, Corked: false}, nil)
		<-p.started
	}
}

but start now looks like this

func (p *PlaybackStream) Start() {
	if p.state.is(idle) { // <------
		p.c.c.Request(&proto.FlushPlaybackStream{StreamIndex: p.index}, nil) 
		p.state.set(running) // <--- between marked calls we have undefined behaviour in other goroutines, because state doesn't changes atomically
		p.err = nil
		p.request <- int(p.createReply.BufferTargetLength)
		p.underflow = false
		p.c.c.Request(&proto.CorkPlaybackStream{StreamIndex: p.index, Corked: false}, nil)
		<-p.started
	}
}

@aykevl
Copy link
Copy Markdown
Contributor

aykevl commented Mar 17, 2025

I made a similar PR a while ago, but didn't make it ready yet. It has a similar approach to this PR.
Code: db8ebac

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.

3 participants