exercism / fsharp

Exercism exercises in F#.
https://exercism.org/tracks/fsharp
MIT License
108 stars 100 forks source link

Add test for concurrency safety in bank-account exercise #260

Closed ErikSchierboom closed 7 years ago

ErikSchierboom commented 7 years ago

The description of the bank-account exercise states that:

Create an account that can be accessed from multiple threads/processes (terminology depends on your programming language). [emphasis mine]

The current test suite does not have any concurrency-related tests. These should be added.

ErikSchierboom commented 7 years ago

The only thing I'm not sure about: how do we create such a concurrency test? The test assumes the object is immutable, so should we do lots of operations on different threads and just check if the original instance is unchanged?

robkeim commented 7 years ago

If the methods are creating immutable objects do you really have potential concurrency issues that we should be testing? Unless someone's implementation uses some mutable, shared state somewhere I don't see what potential concurrency issues you can have.

That being said, your approach is the best I can think of because we don't have an interfaces into the class that we can control (to do things like add artificial delays).

The problem with such a test is that it's going to be non-deterministic which is always frustrating in terms of debugging/validating.

ErikSchierboom commented 7 years ago

Well, the tests don't force you to use immutable data structures, although they do hint towards that solution. That gives you all sorts of benefits, including concurrency safety. I don't think we test more than what I suggested, so let's add that (better than nothing).

robkeim commented 7 years ago

That sounds reasonable!

ErikSchierboom commented 7 years ago

Well, now that I think about it, that's not really what the description implies, is it? It says:

Create an account that can be accessed from multiple threads/processes

The current implementation allows it to be accessed from different threads, but as there is no state, the account is not actually modified. I think this is not what the description tries to tell.

I think we have two options:

  1. Don't use immutability and add the concurrency test in which the account is actually updated.
  2. Use immutability, don't add the concurrency test and perhaps add a hint that we don't do test any concurrency issues in our test suite.

What do you think @robkeim , @rmunn, @jwood803?

robkeim commented 7 years ago

My guess was this exercise was designed for non-functional programming languages which rely more heavily on mutable state. I think it depends what we what to emphasize in this exercise which correspond to the two options you called out above @ErikSchierboom:

  1. Show the power of immutability in terms of not needing to worry about threading/concurrency issues
  2. Simulate "real world" systems where manipulation is happening concurrently

Thread safety isn't something we've touched on at all in these exercises, and we only have the one parallel programming one, so I'd tend to lead towards the second one (and that seems to be what the test description hints at as well). That being said, concurrency and thread safety are advanced programming concepts that aren't adapted for someone just starting out learning a language.

jwood803 commented 7 years ago

That makes sense @robkeim. Though, I wonder if we should so one of the advantages of functional programming and immutability with this exercise. Definitely not for someone starting out, even for me, so is there somewhere we can make a note of that? Maybe in the hint file?

ErikSchierboom commented 7 years ago

If we do make it handle concurrency (which is indeed something that is hardly touched upon), we should increase the difficulty and move it further down the track.

robkeim commented 7 years ago

I wanted to see what other tracks were doing for comparison so here's the list of tracks that have implemented the exercise.

The tracks also rely on some sort of lock/mutex/etc:

Those that don't (or at least I don't understand them enough to see they do):

For those that don't, I don't know if it was an explicit design decision or not.

I'm torn between which direction I think we should go given Exercism's multi-target audience philosophy:

Side note, it was super interesting to see the implementations in a wide variety of languages for the same problem. It gives you a good sense of how verbose a language is just by looking at the lines of code and number of characters on each line.

ErikSchierboom commented 7 years ago

Well, even the Haskell track uses mutation (through the IO monad of course), so maybe we should also have it use mutation.

ErikSchierboom commented 7 years ago

I've just submitted a PR that modifies the bank-account exercise to use concurrency.

ErikSchierboom commented 7 years ago

With #266 merged, this issue can be closed.