data race in state tracking

[StalkR]

Nick isn't safe for concurrent access https://github.com/fluffle/goirc/blob/master/state%2Fnick.go#L15

An example of this issue was reported in StalkR/goircbot#8 where there is a goroutine that checks bot Channels() (a read on Nick), meanwhile the state handling using addChannel (a write on Nick).

I was thinking we could add a mutex in that struct, then protect access to the members (lock/defer unlock).
Thoughts?

[fluffle]

State tracking is potentially racey -- see the other open issue. In current master, state tracker handlers are run strictly before any other handlers for a particular event, and the (default) foreground handlers are run in parallel for that same event while blocking the runloop goroutine. Are you using the "background" handlers? Otherwise the state handlers ought to be completely done with their writes by the time your reads can execute. Or, i've messed something up, which is entirely plausible. I've not actually used the state tracking in anger ever, it was just ... practice.

[StalkR]

It doesn't use any handler, it's a goroutine which shares the *client.Conn, calls Me() to get *state.Nick then Channels() to get the list of channels.

[fluffle]

The idea of scattering locks all throughout this code is making me feel a bit ill, despite all the races inherent in the current design. Some handwaving:

The API presented by the current state tracker is a little ... public, I think. I don't know that external users of the package need the ability to create arbitrary nicks or channels except via the tracker. I was considering making all the tracker methods return struct copies of Nicks and Channels rather than pointers to the originals. More of the actual tracking could be moved into the tracker where fewer, coarser locks would be required, and the copies would represent the state of the tracker at the point the method was called. Mutating them would not affect internal tracker state.

Just a vague idea. Thoughts? I might have some time over Christmas to hack on code ;-)

[StalkR]

+1 on returning copies. You'll still need locks but you can have one RWMutex per struct and the usual lock/defer unlock on methods.

[fluffle]

@lucy @StalkR @stzanpet PTAL at the diff between master and state-copy. There's more to come -- I'm working on the changes to client/. Or, I would be, but a friend bought me KSP for Christmas :-D

[fluffle]

Client changes pushed and tested. Things seem fine. This will probably break code that uses the state tracker, but that's probably also a good thing.

@sztanpet (spelled it wrong last time, sorry)

[sztanpet]

cheers for trying to take everybodies viewpoint into account, and happy new year!
(my code continues to work fine btw, also including me as a contributor is far-fetched but appreciated)

[StalkR]

Cool @fluffle! As expected goircbot breaks, I'll start a branch with the changes like you did and test, then we can merge to our master when ready.

[StalkR]

Done and it seems to works fine. Ready to merge in master when you are!

[StalkR]

ping @fluffle :)

[fluffle]

Sorry, bit busy with life. And, well Minecraft ;-)
On 12 Feb 2015 19:40, "StalkR" notifications@github.com wrote:

ping @fluffle https://github.com/fluffle :)

—
Reply to this email directly or view it on GitHub
#49 (comment).

[TrollWarlord]

You replied but didn't accept the merge?

[fluffle]

Accept what merge? I've not merged my "state-copy" branch into my master yet because I've been busy, as I said. Fortunately for you I've got some time spare this evening ;-)