On Mon, 15 Dec 2025 16:00:30 GMT, David Beaumont <[email protected]> wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/file/Locations.java line
>> 151:
>>
>>> 149:
>>> 150: private void addCloseable(Closeable c) {
>>> 151: synchronized (closeables) {
>>
>> Here and elsewhere -- I'm not sure about `synchronized`. In general, javac
>> operates under the assumption of "single threaded-ness" -- but I see around
>> the file manager code that `synchronized` seems to be used. I'll leave this
>> to @lahodaj
>
> I think in the compiler you can't assume single threaded-ness, and unless
> this adds a deadlock risk (which I am pretty sure it doesn't) it's definitely
> my preference to have correctness and avoid weird bugs/failures.
One instance of javac (i.e. one result of
`ToolProvider.getSystemJavaCompiler().getTask(...)` is single-threaded, and
cannot even be passed to a different thread without an external
synchronization. There's no need for synchronization inside single instance of
javac.
The case of file managers is a bit different: some backend data structures
(like `JRTIndex` as such) are shared across various instances of file managers,
and those definitely need to be synchronized. To what degree synchronization is
needed here is unclear to me. The javadoc says that multi-threaded access to
file managers is not required, and I don't think that concurrent use of the
`JavacFileManager` from multiple instances of javac is quite realistic
(settings like multi-release or preview setting are kept in fields in the file
manager, and the different instances of javac would talk past each other).
I.e. I don't think the synchronization here is really necessary. OTOH, the
`JavacFileManager.getJRTIndex()` is currently synchronized, and it would seem
more consistent for the `getJRTIndex()` and `closeables` to share the same
synchronization approach. I would agree with Maurizio that I would mildly
prefer to drop the synchronization, but I can live with the synchronization.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1761#discussion_r2620083325
