Exi encoding is not thread safe and mix data between threads

[amarant]

Hi,
it seems the encoding in Exi is not thread safe, here is a simple test that shows it :

package com.siemens.ct.exi.main;


import com.siemens.ct.exi.core.CodingMode;
import com.siemens.ct.exi.core.EXIFactory;
import com.siemens.ct.exi.core.EncodingOptions;
import com.siemens.ct.exi.core.FidelityOptions;
import com.siemens.ct.exi.core.exceptions.EXIException;
import com.siemens.ct.exi.core.grammars.Grammars;
import com.siemens.ct.exi.core.helpers.DefaultEXIFactory;
import com.siemens.ct.exi.grammars.GrammarFactory;
import com.siemens.ct.exi.main.api.sax.EXIResult;
import junit.framework.TestCase;
import org.xml.sax.SAXException;
import org.xml.sax.XMLReader;
import org.xml.sax.helpers.XMLReaderFactory;

import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.IOException;
import java.util.*;
import java.util.concurrent.*;

public class ParallelTest extends TestCase {
    public static EXIFactory getExiFactory() throws EXIException {
        GrammarFactory gf = GrammarFactory.newInstance();
        Grammars grammar = gf.createGrammars("./data/XSLT/schema-for-xslt20.xsd");
        EXIFactory exiFactory = DefaultEXIFactory.newInstance();
        exiFactory.setGrammars(grammar);
        FidelityOptions fidelityOptions = FidelityOptions.createDefault();
        fidelityOptions.setFidelity(FidelityOptions.FEATURE_STRICT, true);
        fidelityOptions.setFidelity(FidelityOptions.FEATURE_PREFIX, true);
        exiFactory.setFidelityOptions(fidelityOptions);
        exiFactory.setCodingMode(CodingMode.COMPRESSION);
        EncodingOptions encodingOptions = EncodingOptions.createDefault();
        encodingOptions.setOption(EncodingOptions.INCLUDE_OPTIONS);
        encodingOptions.setOption(EncodingOptions.INCLUDE_SCHEMA_ID);
        exiFactory.setEncodingOptions(encodingOptions);
        return exiFactory;
    }

    public byte[] encodeXmlFileToExi(String path, EXIFactory exiFactory) throws EXIException, SAXException, IOException {
        EXIResult exiResult = new EXIResult(exiFactory);
        ByteArrayOutputStream osEXI = new ByteArrayOutputStream();
        exiResult.setOutputStream(osEXI);
        XMLReader xmlReader = XMLReaderFactory.createXMLReader();
        xmlReader.setContentHandler( exiResult.getHandler() );
        xmlReader.parse(path); // parse XML input
        return osEXI.toByteArray();
    }

    public static final class ExiSch{
        public String Name;
        public String XslPath;
        public byte[] Exi;
        public EXIFactory ExiFactory;
    }

    public void test() throws IOException, SAXException, EXIException, InterruptedException, ExecutionException {
        EXIFactory exiFactory = getExiFactory();
        File dir = new File("./data/XSLT/Examples");
        File[] directoryListing = dir.listFiles();
        HashMap<String, ExiSch> nc1NameExiMap = new HashMap<String, ExiSch>();
        for (File child : directoryListing) {
            byte[] exi = encodeXmlFileToExi(child.getPath(), exiFactory);
            ExiSch exiSch = new ExiSch();
            exiSch.Exi = exi;
            exiSch.Name = child.getName();
            exiSch.XslPath = child.getAbsolutePath();
            exiSch.ExiFactory = exiFactory;
            nc1NameExiMap.put(exiSch.Name, exiSch);
        }
        Collection<Callable<ExiResult>> tasks = new ArrayList<Callable<ExiResult>>();
        for (ExiSch exiSch : nc1NameExiMap.values()) {
            tasks.add(new Task(exiSch));
        }

        ExecutorService executor = Executors.newFixedThreadPool(8);
        List<Future<ExiResult>> results = executor.invokeAll(tasks);
        int errorCount = 0;
        for(Future<ExiResult> result : results){
            ExiResult exiResult = result.get();
            if (!exiResult.valid){
                errorCount++;
            }
        }
        executor.shutdown();
        assertEquals(0, errorCount);
    }

    private static final class ExiResult {
        public boolean valid;
    }

    private final class Task implements Callable<ExiResult> {
        private final ExiSch exiSchData;

        Task(ExiSch exiSch){
            exiSchData = exiSch;
        }
        public ExiResult call() throws Exception {
            byte[] exi = encodeXmlFileToExi(exiSchData.XslPath, exiSchData.ExiFactory);
            ExiResult exiResult = new ExiResult();
            exiResult.valid = Arrays.equals(exi, exiSchData.Exi);
            return exiResult;
        }
    }
}

I added a pull request for the is at #14.
This test use a xsd file : schema-for-xslt20.xsd to construct the grammar, and a list of xsl files in a directory. This is executed with 8 concurrent threads. The generated Exi is different than it should for 8 thread, but not for a single thread (newFixedThreadPool parameter), it is growing with more threads. It also doesn't happen without a grammar. It also doesn't happen with the same file being tested 100 times in parallel. I've also tested with a thread-local ExiFactory it also gives errors.

[danielpeintner]

Hi,

I was aware that the EXIFactory (at least the Grammars) need to be unique across several threads.

The reason is that for finding the right EXI event code for a value "12.34" it needs to be checked first whether the value is compliant with the according datatype (e.g., float). This is a 2 step process.

isTypeValid(Datatype datatype, Value value);
writeValue(QNameContext valueContext);

Anyhow, it looks like that a "local" EXI Factory (with schema-informed EXI Grammars) for each thread causes the same issue.

Let me look into that.

Thanks for reporting this finding!

[danielpeintner]

One more feedback for the time being.

It look like the datatype encoding causes these issues. Setting

fidelityOptions.setFidelity(FidelityOptions.FEATURE_LEXICAL_VALUE, true);

prevents any issue on my side.
The feature "LexicalValue" causes to represent any value as string instead of typed data such as numbers etc.

Can you confirm this situation on your side also?

[amarant]

yes it makes the tests pass.
Also I've added more tests with another pull request here #17
it adds decode test, and add tests with thread local or per task exi factory

[danielpeintner]

Thanks for getting back to us.

Unfortunately I don't see right away what in the datatype handling causes this issue.

Will try to get back to it later this week....

[amarant]

ok, thanks for the solution

[danielpeintner]

Any help/advice with possible threading issues in the code are is useful!

[danielpeintner]

I looked at your updates in ParallelTest (and also fixed on minor issue in exificient-grammars that might have causes threading issues).

I think the SingleFactory tests will fail always as expected.

The ThreadLocalFactory and PerTaskFactory might encounter another issue. I think the XML schema parser library Xerces2-J which is used to load & create EXI grammars is not thread-safe either (nor SAXParserFactory, DocumentBuilderFactory, ....).

Hence, my assumption is that one needs to make sure that first all factories are created in a single-threaded manner. Afterwards all those factories (according to the numbers of threads) should be able to work in parallel.

Does this make sense?

[amarant]

I understand that a Document object for example is not thread safe, I made at one point the test use a single Document concurrently but it used to throw exceptions and I changed the code to only start from the file path. But two different Document used concurrently, each one created from a file path, shouldn't give any problem. The method getExiFactory is static and don't use any static member, it only use the xsd file path so I don't understand how the non thread safety of Xerces objects could cause any problem. I don't think there is a static cache of something in Xerces, or it would mean that nobody could use Xerces using a webserver.

[danielpeintner]

I added some more tests to ParallelTest w.r.t. loading XSDs or EXI grammars in parallel. The results seem to be fine.

Anyhow, somewhere in the chain instances of com.siemens.ct.exi.core.datatype.Datatype seem to be re-used... Currently I just don't understand where?
I think this is the only explanatation why

fidelityOptions.setFidelity(FidelityOptions.FEATURE_LEXICAL_VALUE, true);

works.

[amarant]

I think I found the problematic datatype ... DEFAULT_DATATYPE in BuiltIn.java, if you just replace it with a static getter that return a new Datatype each time the error for testParallelEncodeWithThreadLocalFactory and testParallelEncodeWithPerTaskFactory vanish, but not for testParallelEncodeWithSingleFactory.

[amarant]

It makes a lot of test failing because there is a lot of part of the code using reference equality instead of the overrided equals method.

[danielpeintner]

Thank you for finding this issue!

I updated the code as suggested and I do see now only failures for the 2 SingleFactory test-cases which I expected. The reason is that with a single factory the grammars and also its datatypes are shared across multiple threads. The Datatype class has 3 methods that should not be handled as part of the class but rather external to it.

	/*
	 * Encoder
	 */
	public boolean isValid(Value value);

	public void writeValue(QNameContext qnContext, EncoderChannel valueChannel,
			StringEncoder stringEncoder) throws IOException;
	/*
	 * Decoder
	 */
	public Value readValue(QNameContext qnContext, DecoderChannel valueChannel,
			StringDecoder stringDecoder) throws IOException;

I think this should resolve all issues.
Do you agree?

[amarant]

Yes I agree, also Datatype classes, that belong to grammars, shouldn't keep values in them as fields.

[amarant]

The writeValue method should take a value and not use the last stored value.

[danielpeintner]

The writeValue method should take a value and not use the last stored value.

The reason is to avoid parsing a string-based value a second time on encoder side.

Having said this, I think this part of the code needs to be moved to TypeEncoder (or respectively TypeDecoder) anyway. This is a bit more work and I will try to get to it the next days...

[amarant]

Maybe there could be a method writeValueIfValid that returns a boolean if it is valid and do the write, or else return false and don't do it, but maybe there are some intermediate work between the two sometimes ? Anyway moving this code to TypeEncoder that are owned by the Exi Encoder is the right idea.

[danielpeintner]

I moved readValue(...) from class Datatype to TypeDecoder and isValid(...) and writeValue(...) from class Datatype to TypeEncoder.

I believe this should resolve the issue and EXIGrammars can be shared across parallel threads. The test-cases don't show further issues either. May I ask you to check and provide feedback. Thanks!

[amarant]

Yes it seems to fix all the issues I had. Thanks a lot.