#640 Loading models into one Viewer from several threads corrupts the model set (minimal sample + patch)

[Amr ALHOSSARY]

Hi Bob,

A while back you asked me for a minimal crashing sample and I never managed to produce one. I have one now: a single file, no data files, no GUI, no third-party libraries. It builds a poly-alanine model in memory and calls viewer.openStringInline() on one JmolViewer. It is attached as JmolThreadCrash.java.

javac -cp Jmol.jar JmolThreadCrash.java
java  -cp Jmol.jar:. JmolThreadCrash

On Jmol 16.4.1 with Java 26.0.1 (Linux x86-64):

• 60 loads on a single thread: 0 failures.
• The same 60 loads spread over 4 threads: 12 failures.
• 14.31.10 behaves the same way (16 failures), so this is long-standing rather than a regression.

Hunt.java (also attached) is the same test with every throwable recorded and deduplicated by exception class plus top frame. Stock 16.4.1, 600 loads over 4 threads, gives 110 failures across 7 distinct signatures:

 33 x NullPointerException @ org.jmol.modelsetbio.BioResolver.setGroupLists(BioResolver.java:895)
  7 x NullPointerException @ org.jmol.modelsetbio.BioModel.setBioModel(BioModel.java:96)
  4 x NullPointerException @ org.jmol.modelset.ModelSet.calculateStructuresAllExcept(ModelSet.java:4459)
  4 x NullPointerException @ org.jmol.modelset.Model.getChain(Model.java:292)
  1 x ConcurrentModificationException @ java.util.Hashtable$Enumerator.next
  1 x ArrayIndexOutOfBoundsException @ javajs.util.BS.clearAll(BS.java:360)
 60 x NullPointerException @ (no stack trace)

That last line is the JVM's fast-throw optimisation after repeated NPEs at the same site - worth knowing, because anyone hitting this in the wild sees stackless exceptions and no clue where they came from.

I want to be clear that I am not reporting this as "Jmol should be thread-safe". I understand the Viewer is meant to be driven from one thread, and I have already fixed my own application (CrossLinX) by funnelling every viewer call onto the Swing event thread, which takes the failure count to zero. I am reporting it because the way it fails is expensive to diagnose: the exception surfaces deep inside Jmol with no indication that the caller did anything wrong, and in my original case it arrived on Jmol's own ScriptQueueThread with no frame of mine in the stack at all:

java.lang.NullPointerException: Cannot read field "vwr" because "modelSet" is null
    at org.jmol.modelsetbio.BioModel.<init>(BioModel.java:82)
    at org.jmol.modelsetbio.BioResolver.getBioModel(BioResolver.java:139)
    at org.jmol.modelset.ModelLoader.setModelNameNumberProperties(ModelLoader.java:608)
    ...
    at org.biojava.nbio.structure.align.gui.jmol.JmolPanel.setStructure(JmolPanel.java:156)

Anything along these lines would have saved me a lot of time, in increasing order of effort:

1. A sentence in the JmolViewer API docs saying a viewer instance must be driven from a single thread.
2. Failing fast - if a load begins while another is in progress on a different thread, throw something that names the problem.
3. Making a load atomic per viewer, so a concurrent load waits rather than observing a half-built model set.

(1) alone would be worth it.

I did try (3), and jmol-thread-safety.patch is attached in case it is useful. Two files, five short wrappers, no existing method body changed. Everything takes one monitor, the per-Viewer ModelManager instance: ModelManager.zap and ModelManager.createModelSet become synchronized, and Viewer.zap, Viewer.createModelSetAndReturnError and Viewer.openStringInlineParamsAppend each hold synchronized (mm) while delegating to their original body. I chose that monitor because ModelManager is per-Viewer and both new ModelLoader(...) calls live inside it; monitors are reentrant, so the nesting is fine, and synchronized is a no-op under SwingJS.

Getting there took three passes, and the middle one may be the interesting part for you:

• Synchronizing ModelManager alone took failures from 12 to 1, with the survivor in ShapeManager.shapes via setFrankOn.
• Adding Viewer.createModelSetAndReturnError left a rarer one still: 2 failures in 2400 loads, a ConcurrentModificationException in GlobalSettings.clear under StateManager.clear under Viewer.zap.
• That pointed at the real shape of it: a load is zap-then-create, since openStringInlineParamsAppend calls zap(...) and then createModelSetAndReturnError. Locking the halves separately still leaves the pair non-atomic, so the lock has to span both.

Results with the patch: 0 failures in 2400 loads at 4 threads, and 0 in 6000 loads at 8 threads, against 110-in-600 for stock. Cost: single-threaded load time is unchanged (34.3 vs 33.9 ms/load, inside the noise of my measurement - an uncontended monitor cannot cost anything against a 34 ms load), and concurrent loads now run at the single-threaded rate because they are serialised, which is the intent.

I would not claim this makes the Viewer thread-safe. It covers the load and zap paths, and other Viewer-wide state is reachable from elsewhere. You are far better placed than I am to judge whether that lock belongs there at all, or whether (1) and (2) are the better answer.

To build and test the patch without the full ant build:

tar xzf Jmol-16.4.1-full.tar.gz
cd jmol-16.4.1 && patch -p1 < jmol-thread-safety.patch
javac -implicit:none -cp Jmol.jar -sourcepath src -d /tmp/patched \
    src/org/jmol/viewer/Viewer.java src/org/jmol/viewer/ModelManager.java
java -cp /tmp/patched:Jmol.jar:. JmolThreadCrash

(-sourcepath is needed because javajs.J2SIgnoreImport ships in source only, not in Jmol.jar.)

Thanks for Jmol - it has been the backbone of this project for years.

Amr

[Bob Hanson]

I have incorporated jmol-thread-safety.patch into Jmol-SwingJS and Jmol manually. Thank you VERY much for this. I think it might also solve some problems I have seen but not been able to track down when loading new models while threads such as spin threads are running. Fixed for Jmol 16.4.21/22