On Sat, 11 Jun 2022 07:55:52 GMT, Peter Levart <plev...@openjdk.org> wrote:

>> Oops, yes you are correct!
> Hi,
> 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...

Well, studying this further, I think some races are already accounted for and 
are harmless except one. The `handle` method 1st attempts to register handler 
in native code via call to `handle0` and then updates the `signals` map.. As 
soon as native code registers the handler, `dispatch` may be called and the 
lookup into `signals` map may return null which would cause NPE in the 
following lookup into `handlers` map. So there are two possibilities to fix 
- guard for null result from `singnals` lookup, or
- swap the order of modifying the `signals` map and a call to `handle0` native 
method. You could even update the `signals` map with `.putIfAbsent()` to avoid 
multiple reassignment with otherwise equal instances. This may unnecessarily 
establish a mapping for a Signal the can later not be registered, so you can 
remove it from the `signals` map in that case to clean-up.
The possible case when the lookup into `handlers` map returns null Handler is 
already taken into account, but there is a possible case where a NativeHandler 
is replaced with a java Handler implementation and `dispatch` is therefore 
already called, but `handlers` map lookup still returns a NativeHandler. In 
that case the NativeHandler::handle method will throw exception. To avoid that, 
NativeHandler::handle could be an empty method instead.


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

Reply via email to