Comments (4)
the whole
Write
function is not thread-safe
Well, this problem was long before my time, but if I look at it, I say yes it's true that Write
is not thread safe... which actually is written in the documentation for it:
https://github.com/exercism/xgo/blob/d895e12bc457dad8c024ad457380ade2a5f55aa9/exercises/paasio/example.go#L11
https://github.com/exercism/xgo/blob/d895e12bc457dad8c024ad457380ade2a5f55aa9/exercises/paasio/example.go#L20
https://github.com/exercism/xgo/blob/d895e12bc457dad8c024ad457380ade2a5f55aa9/exercises/paasio/example.go#L30
Furthermore if this function is going to have race problems they're most likely going to be in the buffer write since incrementing an integer is so quick and costs so little time relatively to the buffer write that you need really bad luck to hit it.
Well, the tests do check that the counters are correct. And if you remove the lock/unlock, you actually get incorrect results. There are so many goroutines doing the reads/writes simultaneously. So we have many chances to get really bad luck... so it turns out we actually need really good luck to not see errors.
Putting the
wc.w.Write
within the critical section is possible
Yeah so as I understand it, it is altogether likely that the contents written to the buffer are complete garbage because of it not being in the critical section.
So I guess the tests are kind of saying "even though the calls to Write()
are not synchronized and therefore the customer who used Write()
might have written garbage data, you still need to charge your customer for all the garbage data that was written.".
Is that an OK thing to say? I don't know for sure. It's possible that we could make that clear in the docs. For example, to change "calls to Write()
are not synchronized" to something like "if multiple goroutines call Write()
simultaneously, no guarantee is made about the contents written to the underlying Writer
(unless that Writer
provides any such guarantees), but the counters will still correctly reflect all operations performed."
as I understand the point of the created code is to provide io.Reader and io.Writer wrappers that themselve satisfy io.Reader and io.Writer respectively. In that respect it's a bit odd that the Read and Write methods would be thread-safe if the interface doesn't require it.
I think that if you are implementing an interface there's nothing wrong with providing extra guarantees above and beyond what the interface requires of you. Should probably document such extra guarantees though (which is why I argue to change the documentation on the functions).
But fundamentally I agree that there is no need for our WriteCounter
, etc. to synchronise access to their Writer
s.
Another issue I see with this code is that it's about mutexes and those are somewhat discouraged in Go. They're around for when you need them but you shouldn't use them unless you have a good reason for why channels won't do the job. So is this something we want to teach to newbies?
I'm a little undecided. Sure, we could have all the counters send byte counts onto a channel and have a single goroutine that reads out of the channel and updates the counter appropriately. No mutex needed then. One disadvantage of this approach I see is that if we no longer need a Reader, we now have no way of telling the goroutine to exit, so it's forced to stick around forever. There's no Close
method on these readers. Now we could solve this problem by adding Close
methods...
I may take a stab at implementing the channel-based approach and see how it looks.
from go.
As you may have seen, I decided it was best to let the code do the talking, so I implemented a channel-based version of this exercise at #236 and gave my comments there. My general opinion is that for this specific use case I think mutexes are more appropriate than channels and I think we shouldn't be afraid to show that to learners, but I'd gladly take discussion there.
from go.
Given a few factors:
- The concurrency guarantees are now documented
- I believe the mutexes are OK
- Issue's pretty old
I think we have done all we can here and can close, but obviously further discussion is always welcome, here or in another ticket.
from go.
Sounds good.
from go.
Related Issues (20)
- The Farm: Tests do not cover a condition of the exercise HOT 1
- Go track exercise elons-toys passes tests in cases where it should not
- Test error HOT 1
- Fails HOT 2
- Improve test output for the Clock exercise
- Enable test runner v3 UI for all concept exercises
- Ensure package names are unique
- Adding concurrency concepts to the syllabus HOT 3
- Community contributions are paused for this repo
- Outdated information in the 'Randomness' section HOT 1
- ErrStop and ErrInvalidBase not defined in the test file. HOT 1
- Booking up for Beauty: Better explanations for tasks HOT 1
- Update go version to 1.21 in go.mod across all or subset of problems? HOT 1
- Remove check for short test in benchmarks
- Hello world: Error: not in workspace HOT 1
- Building a training set of tags for go HOT 22
- Bug: instructions for kindergarten_garden do not match requirements HOT 2
- Define more test cases for resistor color HOT 1
- Expanding test cases for Vehicle Purchase exercise in the Go track HOT 1
- Need for speed exercise CanFinish tests do not validate start with distance travelled HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from go.