It's pretty unusual to
declare all of your functions before defining them.
It appears that you did it so that you could define your functions in the order you thought
most appropriate. That's common for newcomers, and it's certainly something many people are
used to. To be clear, there's nothing wrong with wanting that.
However, most clojure programs work within this constraint and just define things in the
order they are needed. After a while you become accustomed to seeing things in that order,
and you start to look at the bottom of the file for the program overview. This has the
minor benefit of calling out the places where circular references are actually needed.
This is a toy program, so calling
-main in your namespace probably does what you want 100%
of the time. However, in a real codebase, doing that can pretty much make your namespace
unusable for other namespaces. If you're using
lein you can call:
lein run -m concurrency.dining-philosophers
If you're running raw clojure use:
java -cp "clojure.jar:$OTHER_JAR_PATHS" clojure.main -m concurrency.dining-philosophers
Your philosophers are aware of a lot of implementation details. Really all they care
about is their name, their left fork, and their right fork. You transmit all of that information
in a single number, which is convenient, but it means you have to be concerned with how
forks are retrieved in the
philosopher code as opposed to just using the forks you're assigned.
The consequence of this is it binds your functions together in an undesirable way.
Your solution uses a lot of indices and math to manipulate those indices. It could be made
dramatically simpler by manipulating sequences instead. (Hint: Use the above advice and the
This is a really common thing for newcomers to clojure, myself included.
When I did a lot of Java, I tended to like very small functions, and I would create arbitrary
wait-retry-time) for no other reason than to cut
down on my line count. After working with clojure for a while, function length became less important
than overall comprehensibility. So what I try and do now is look for fundamental concepts in my
program as opposed to places where I can inject a random function.
For example, for this problem, I see
think as primitives.
They are things that are inherent to the problem. You may or may not name them, but they are definitely
You never really want to get a single fork at one time, so
get-fork seems too small to be a standalone
concept. The same for
wait-retry-time is just a pseudonym for
(Thread/sleep retry-time). The latter is going
to be more obvious to any newcomer to the codebase.
Use of core.async
Just to let you know, since you pulled in core.async, it's possible to do a lot of the
mechanics around starting/stopping threads with core.async. This has a couple of advantages:
- Removes the need to use Java constructs like
.wait. (As far as I know, this is really just an aesthetic advantage)
- Allows you to easily interact with processes (i.e. start them and stop them) (Another potential option here is Java's Executor
So it's not a massive gain, but the option is there if you choose.
No cleanup mechanism
While we're on the topic of interacting with the processes, the current structure doesn't
leave any hooks for cleaning up your threads. This works fine for a toy program where you'll
let it run then ctrl-c, but any real-world system will need to handle that
cleanly. So it might be nice for you to use the opportunity to think about how to structure the
program so you can clean up on demand.
logger-chan is an unbuffered
chan you've introduced synchrony around your logging
when you probably want some asynchrony there. I can't see any harm in setting a buffer of ~100 or
~1000 on that channel. It'll almost certainly be way ahead of its workload, and in the event it gets
behind, it'll still give some backpressure so you won't lose any events.
I really like that you chose clojure's STM system for this problem. It's not often used, but by
far the best fit. Also, despite my nitpicking, the big picture flow of
philosopher is straightforward
in the best kind of way.
Please let me know if you have any questions about my comments. I'm more than happy to give further feedback!
Edit in response to OPs comments
You only get lock up if you occupy the thread (e.g. with
Thread/sleep, or by doing IO or doing a CPU-intensive process). If you "park" it with
(<! (a/timeout msecs)) it doesn't block the state machine.
core.async vs future
I was referring to the
thread macro. I don't think there's any substantial benefit to core.async over the futures with and
Executor. I actually did a version with
Future, and it has the same feature set. FWIW I think it's also more concise than the version I did with core.async.
However, it is very beneficial to use one of those two over
Thread/wait. Those are pretty rudimentary constructs. And they suck for most use cases.
You pose a really good point regarding
get-fork. You do indeed want to eliminate repetition where possible (though not at the expense of simplicity). I would agree that in the current implementation,
get-fork needs to be its own function. However, I don't think getting a single fork is necessary for a solution to this problem.