XMLNode.convertStringToXMLNode is not thread safe

[piotr-gawron]

The problem mentioned in the title is visible when in few threads you try to assign notes (as String) into AbstractSBase object. The AbstractSBase can be a model (different model in every thread) or a species. Below is the code that shows the behavior - every thread creates its own model (it's using 20 threads, but if you are lucky you can see it on 2):

Set<Thread> threads = new HashSet<>();
MutableBoolean exceptionHappened = new MutableBoolean(false);
for (int i = 0; i < 20; i++) {
  Thread thread = new Thread(new Runnable() {
    @Override
    public void run() {
      SBMLDocument doc = new SBMLDocument(3, 2);
      Model result = doc.createModel();
      String rdf = "<rdf:RDF xmlns:rdf=\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\" xmlns:dc=\"http://purl.org/dc/elements/1.1/\" xmlns:dcterms=\"http://purl.org/dc/terms/\" xmlns:vCard=\"http://www.w3.org/2001/vcard-rdf/3.0#\" xmlns:bqbiol=\"http://biomodels.net/biology-qualifiers/\" xmlns:bqmodel=\"http://biomodels.net/model-qualifiers/\">\n"              +
          "<rdf:Description rdf:about=\"#TestGEN\">\n" +
          "<dcterms:modified rdf:parseType=\"Resource\"></dcterms:modified>\n" +
          "</rdf:Description>\n" +
          "</rdf:RDF>";
      try {
        result.setAnnotation(rdf);
      } catch (Exception e) {
        e.printStackTrace();
        exceptionHappened.setTrue();
      }
    }
  });
  thread.start();
  threads.add(thread);
}
for (Thread thread : threads) {
  thread.join();
}
assertFalse(exceptionHappened.booleanValue());

The problem that I was able to identify is here: https://github.com/sbmlteam/jsbml/blob/master/core/src/org/sbml/jsbml/xml/XMLNode.java#L116 - SBMLReader is static, but the class is stateful. So when one thread access the reader it enters some state that is overridden by another thread and as a result XMLNode.convertStringToXMLNode can return null instead of meaningful result.

The easiest solution would be to use new SBMLReader with every call or make all calls to the notesReader methods synchronized.

[niko-rodrigue]

Thanks for the bug report. I think the static SBMLReader was added a long time ago to try to speed reading of models but I think it is not necessary any more.

I will do some speed tests and integrate the fix if it is fine.

[niko-rodrigue]

I have done a fix in master as well as deployed this fix to the maven artefact JSBML-1.5-SNAPSHOT. Could you test it to see if everything is fine now.

[piotr-gawron]

I can confirm that the issue is fixed.

Thanks