On Fri, 10 Jun 2022 11:45:28 GMT, David M. Lloyd <d...@openjdk.java.net> wrote:

>> Hello David, I suspect you mean `handlers.put(sig, handler)` and not 
>> `handlers.replace(...)`? And yes, I think what you suggest will help remove 
>> the synchronized block here.
> Oops, yes you are correct!

I think the synchronized block was redundant already in original code. Since 
the entire handle method is `static synchronized` and it is the only method 
that modifies the `handlers` and `signals` maps.
But even with so much redundant synchronization, the Signal class is not 
without races. There are multiple possible races in `dispatch(int number)` 
method which is called from native code to dispatch a signal:
- race no. 1: dispatch method (not synchronized) performs 2 independent lookups 
into `signals` and `handlers` maps respectively, assuming their 
inter-referenced state is consistent. But `handle` method may be concurrently 
modifying them, so `dispatch` may see updated state of `signals` map while 
seeing old state of `handlers` map or vice versa.
- race  no. 2: besides `signals` and `handlers` there is a 3rd map in native 
code, updated with `handle0` native method. Native code dispatches signals 
according to that native map, forwarding them to either native handlers or to 
`dispatch` Java method. But `handle` method may be modifying that native map 
concurrently, so dispatch may be called as a consequence of updated native map 
while seeing old states of `signals` and `handlers` maps.

I'm sure I might have missed some additional races.

How to fix this? Is it even necessary? I think that this internal API is not 
used frequently so this was hardly an issue. But anyway, it would be a 
challenge. While the two java maps: `handlers` and `signals` could be replaced 
with a single map, there is the 3rd map in native code. We would not want to 
make `dispatch` method synchronized, so with careful ordering of modifications 
it is perhaps possible to account for races and make them harmless...


PR: https://git.openjdk.org/jdk/pull/9100

Reply via email to