WatchService - Exposing more of the Inotify Event Model

[Benedict Elliott Smith]

The problem with this solution is that it is not intuitive and its necessity
and added complexity can be completely removed by a simple change to the API
to make it more intuitive (and hence generally safer).

IMHO the number one aim of public APIs like this should be simplicity and
safety of use. With all APIs there turn out to be common mistakes/pitfalls
in using them that become apparent over time, but in this case there is a
chance to mitigate this particular potential pitfall by a simple
modification to the API itself, prior to it being widely used. This seems to
me like it can only be a good thing.



On 23 September 2010 19:12, Rémi Forax <forax at univ-mlv.fr> wrote:

>  Le 23/09/2010 19:15, Benedict Elliott Smith a écrit :
>
> On 23 September 2010 18:11, Rémi Forax <forax at univ-mlv.fr> wrote:
>
>> Le 23/09/2010 17:45, Benedict Elliott Smith a écrit :
>>
>> It *is* fairly easy to store the Watchable as a field in the WatchKey,
>> except that the WatchKey is created by the WatchService, over which the user
>> typically has no control - and the default  implementations return
>> a WatchKey that cannot tell you what it is watching (despite obviously
>> having to know this).
>>
>>
>>  Ok.
>> As I said, I think a more general attached Object is a better solution in
>> my opinion.
>>
>>
> If you would rather provide the ability to annotate the information
> WatchKey with arbitrary information this would be a perfectly acceptable
> solution IMHO.
>
>>
>>  As such, currently (and in the example usages) the user has to insert
>> the WatchKey that is returned by the WatchService into a map which stores
>> the Watchable. The problem is that if a different thread registers the
>> Watchable to the one consuming events from the WatchService, it is possible
>> for a WatchKey to be returned to the consumer before the map contains it,
>> and hence the consumer does not know what Watchable it is for.
>>
>>
>>  If you store/retreive the path in a synchronized block or use a
>> ConcurrentHashMap
>> and call put() before registering, I don't see why there is a problem ?
>>
>>
> Using a ConcurrentHashMap only enforces a happens-before relation, it does
> not address race conditions. Consider the following pseudo-code:
>
>  Thread1 {
>     watchkey = watchservice.register(mypath)
>     sharedmap.put(watchkey, mypath)
> }
>
>  Thread2 {
>     watchkey = watchservice.poll()
>     path = sharedmap.get(watchkey)
> }
>
>  While making sharedmap either synchronized or a ConcurrentHashMap does
> guarantee that the map is consistent and that the mapping eventually shows
> up for Thread2, it does nothing to address the fact that Thread1 may be
> swapped out after executing the first instruction, and not execute its
> second instruction until after Thread2 reads from the map, getting no
> result.
>
>  You could synchronize around both steps, but this is really not a very
> elegant solution. Also, it is a pitfall many people will not even notice,
> and hence not code to avoid. It seems best to address it with an obvious and
> simply solution that is built into the standard examples of usage. i.e. that
> the path is obtainable from the WatchKey. It seems likely that this
> information will required more often than not, surely?
>
>
> There is another possible implementation, using a blocking queue and
> interrupt:
>
> Thread {
>     queue.put(mypath);
>     watchserviceThread.interrupt();
>  }
>
>  WatchserviceThread {
>     for(;;) {
>       try {
>          watchkey = watchservice.take()
>       } catch(InterruptedException e) {
>          while ((path = queue.poll()) != null) {
>            path.register(watchservice);
>            sharedmap.put(watchey, path);
>          }
>          continue;
>       }
>        path = sharedmap.get(watchkey);
>       ...
>     }
>  }
>
> In that case, the registration and the watchkey retreival are
> confined in one thread.
>
>
>
>
>>  This seems bad to me.
>>
>>  Even if this weren't the case though, it seems a bit unnecessary to
>> expect the user to store the Watchable themselves - as you say, it should be
>> easy to store it in the WatchKey, so I'm not sure why it isn't exposed to
>> the user? Or am I missing something obvious?
>>
>>
>>  It's not exposed because the current implementation doesn't store it :)
>> On Linux the file descriptor is stored not the corresponding Path.
>>
>>
>  Fair enough, I can see why you would have chosen to avoid putting it in
> (since Path is not a particularly lightweight object) - but it seems like it
> might be a necessary evil to help prevent this problem biting users randomly
> without their knowing why or what to do about it.
>
>
>> Rémi
>>
>
> Rémi
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20100923/6ec79159/attachment.html