UDS: make writer able to deal with concurrent writes #62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit will make the CI fail, as this demonstrates utilizing the
UDS writer in a concurrent program is currently unsafe.
This is triggered because the current UDS class changes the underlying
`conn` pointer on errors unsafely.
This would show up as:
```runtime error: invalid memory address or nil pointer dereference```
Running the `TestClientUDSConcurrent` test with the race detector shows
this as well.
```
==================
WARNING: DATA RACE
Read at 0x00c420126000 by goroutine 16:
[...]
github.com/DataDog/datadog-go/statsd.(*udsWriter).Write()
/Users/leo/datadog/go/src/github.com/DataDog/datadog-go/statsd/uds.go:52 +0x14e
[...]
Previous write at 0x00c420126000 by goroutine 13:
[...]
github.com/DataDog/datadog-go/statsd.(*udsWriter).Write()
/Users/leo/datadog/go/src/github.com/DataDog/datadog-go/statsd/uds.go:46 +0x328
[...]
```
There is a failing test for this usecase ebb1a50 Adding this mutex allows for making sure when a write happens and can decide to change the `conn` attribute (to create a new one or to replace a failing one) it locks the resource.
LeoCavaille
changed the title
|
We tried this change and it killed performance. I would revert since it makes this library usable. |
|
Question for @nabiber and @LeoCavaille - AFAIK concurrent |
KJTsanaktsidis
added a commit
to zendesk/datadog-go
that referenced
this pull request
Dec 11, 2018
PR DataDog#62 added in a `Mutex` around the Unix socket writer's `Write` method, to protect the logic that conditionally connected the socket if there was not one already. However, because of the lock placement, it serialises all statsd writes globally, which is likely to be a performance bottleneck for highly parallel systems. This commit turns that Mutex into a `RWMutex`. In the case where there is already a socket set, only a `RLock` is needed. In the case where the socket needs to be connected, the a full `Lock` is performed to protect the swapping of `w.conn`. Note that this change, I believe, will make `SetWriteTimeout` meaningless; multiple goroutines will call `conn.SetWriteDeadline` in parallel, and the behaviour of that is to extend the deadline for future _and all in-flight_ requests. If new calls to `SetWriteDeadline` keep getting made, the deadline on an existing blocked call to `Write` will be continuously extended. I'm not really sure how to fix this or whether it's worth fixing though.
truthbk
added a commit
that referenced
this pull request
Apr 8, 2019
…84) * uds: chage Mutex to RWMutex for fast already-connected path PR #62 added in a `Mutex` around the Unix socket writer's `Write` method, to protect the logic that conditionally connected the socket if there was not one already. However, because of the lock placement, it serialises all statsd writes globally, which is likely to be a performance bottleneck for highly parallel systems. This commit turns that Mutex into a `RWMutex`. In the case where there is already a socket set, only a `RLock` is needed. In the case where the socket needs to be connected, the a full `Lock` is performed to protect the swapping of `w.conn`. Note that this change, I believe, will make `SetWriteTimeout` meaningless; multiple goroutines will call `conn.SetWriteDeadline` in parallel, and the behaviour of that is to extend the deadline for future _and all in-flight_ requests. If new calls to `SetWriteDeadline` keep getting made, the deadline on an existing blocked call to `Write` will be continuously extended. I'm not really sure how to fix this or whether it's worth fixing though. * [rebase] moving uds to uds_blocking for rebase * [uds][blocking] fixing bad struct name after rebase
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Each commit message has more info.
I added a failing test that highlighted the issue first before adding the code to fix it.
The CI already runs with the race detector enabled so it should be able to test the change properly.