scala-ide/scala-ide — pull request #241 by dotta (2012-12-03 17:23:19+00:00)
Title: Issue/race condition in event dispatcher

This PR was supposed to fix #1001360, but it turns out there was nothing to fix. But have a look at the last commit (please ignore the other commits), and let's see what you guys think.

Here is the commit message:

This was supposed to be a fix for a race condition, but as suggested by
@skyluc, there was actually no race condition to fix in the first place.
Nevertheless, I carried on the refactoring because I believed it would have
produces code that is easier to reason about. Well, I was wrong again.

But, before getting into the details of why I think the performed refactoring
is suboptimal, let's first give an intuition of why the issue described in
refactoring didn't produce the expected improvement.

The not so real race condition
==============================

As described in the ticket, I was under the (wrong) impression that the
following code

```scala
val request = createSomeRequest()
eventDispatcher.setActorFor(someActor, request)
request.enable()
```

could be subject to lost updates. The reasoning was the following:

Calling `eventDispatcher.setActorFor(someActor, request)` results in an
**asynchronous** message being sent to the eventDispatcher's companion actor,
hence it is possible that `request.enable()` is enabled **before**
eventDispatcher's companion actor gets a chance to process the
`SetActorFor` message.

However, this is simply not a problem!

Why? Because even if the `request` is enabled before the `SetActorFor` is
processed by the `ScalaJdiEventDispatcher`'s companion actor, any event sent
from the `ScalaJdiEventDispatcher` to its companion actor will **always** be
processed **after** the the actor has handled the `SetActorFor` message.
(Read the above paragraph a couple of times, and will hopefully make sense)

Basically, we can rely on the *happen-before relation* between the sending of
two messages from **the same** actor.

The not so good refactoring
===========================

Let's now have a look at the refactoring carried out in this commit.

An `EventRequest` contains two methods, `putProperty` and `getProperty`, which
can be used to link (and unlink) the actor responsible of handling a specific
event, with its (event-)request. The issue of using the `EventRequest` map
is that it's not thread-safe! Which is a big problem, since the map is going
to be accessed by different threads. Wrapping accesses to the map in a
`synchronized` block can help to some extent, but it still feels like a inferior
solution to what we had before.

In short, the refactoring didn't make the code any simpler to reason about. And,
to be honest, I have a feeling that now the logic is potentially wrong.

Conclusion
==========

Bottom line, this was an interesting exploration, but I would actually suggest
to keep the existing code and add a few comments to explain why it is correct.