Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

fix: add read/write locks #85

Merged
merged 1 commit into from
Apr 22, 2020
Merged

fix: add read/write locks #85

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions conn_native.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,14 @@ type Conn struct {
DefaultMessageType int
reader io.Reader
closeOnce sync.Once

readLock, writeLock sync.Mutex
}

func (c *Conn) Read(b []byte) (int, error) {
c.readLock.Lock()
defer c.readLock.Unlock()

if c.reader == nil {
if err := c.prepNextReader(); err != nil {
return 0, err
Expand Down Expand Up @@ -67,6 +72,9 @@ func (c *Conn) prepNextReader() error {
}

func (c *Conn) Write(b []byte) (n int, err error) {
c.writeLock.Lock()
defer c.writeLock.Unlock()

if err := c.Conn.WriteMessage(c.DefaultMessageType, b); err != nil {
return 0, err
}
Expand Down Expand Up @@ -113,10 +121,18 @@ func (c *Conn) SetDeadline(t time.Time) error {
}

func (c *Conn) SetReadDeadline(t time.Time) error {
// Don't lock when setting the read deadline. That would prevent us from
// interrupting an in-progress read.
return c.Conn.SetReadDeadline(t)
}

func (c *Conn) SetWriteDeadline(t time.Time) error {
// Unlike the read deadline, we need to lock when setting the write
// deadline.

c.writeLock.Lock()
defer c.writeLock.Unlock()
Copy link

@0xKiwi 0xKiwi May 14, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend unifying these into a sync.RWMutex and use lock.RLock() in combination with lock.Lock(). Since the write lock is set without setting the read lock.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're locking reads independently from writes. The underlying library does not allow concurrent reads from multiple threads or concurrent writes from multiple threads.

However, reading while writing (as long as only one thread is reading reading and only one thread is writing) is perfectly fine. As a matter of fact, that's how we normally use connections.

Note: the specific bug we were fixing here was concurrently setting a deadline while writing. We could have simply locked the write side and that's it, but we've run into hard to track down race conditions in the past where we were reading from multiple threads without synchronizing (usually in error paths where we're throwing away the connection anyways).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Thank you for the explanation!


return c.Conn.SetWriteDeadline(t)
}

Expand Down