Using DDMSence in a multi-threaded environment

[Brendan Hofmann]

We're using DDMSence in a multi-threaded OSGi enviroment to work with parsing, validating, and transforming between multiple versions of DDMS. It seems that DDMSence is not thread-safe due to the singleton pattern of DDMSVersion, when working with multiple versions of DDMS simultaneously. I know that the docs specifically say that no special effort has been made to make DDMSence thread-safe, but this is a performance requirement for us. This leads me to two questions:

1. I'm looking at possible ways to make DDMSence thread-safe. What was the reason for the singleton pattern of DDMSVersion? I can see that it makes the rest of the codebase simpler. Are there performance reasons or other technical reasons why this approach was used as well? Any feedback you can offer on possible solutions or potential complications would be helpful. So far the DDMSVersion singleton is the only thread-safety issue I've found.
2. Are you willing to consider addressing thread-safety in future versions? I'd be happy to help. If fixing this issue is a viable alternative, those improvements would be submitted back to the project for your consideration, of course.

[Brian Uri!]

Hello Brendan,

The DDMSVersion _currentVersion singleton should be the only part of the library where state is handled in a thread-unsafe manner. Class immutability trumped thread safety in my original design: I wanted the versioning mechanism to be as unobtrusive as possible to library users, while maintaining the immutability of each component class. Maintaining the current version in a non-static way would have added additional required parameters to all component constructors and builders, and at the time, I felt that many of the constructors were already too verbose. So, this approach is more about simplicity and usability than performance or other technical reasons.

Here are the technical details of the specific workflows where this might get complicated in a multithreaded environment:

1) Reading DDMS metacards from an input stream via DDMSReader
Current Impact: Because each DDMSReader is instantiated with a specific DDMSVersion, and because you can have multiple readers active at the same time, reading metacards should always be thread-safe. You should not need to worry unless you take a successfully-read metacard and drop it into a Component Builder for transformation / manipulation (see #4 below).

2) Building components with XML-based constructors ("bottoms-up")
Current Impact: Because these constructors use the XML namespace of the XML elements to determine the correct version, these constructors should be thread-safe.

3) Building components with data-based constructors ("bottoms-up")
Current Impact: Each constructor that builds a DDMS component from basic Java data types relies upon the _currentVersion singleton at the moment of instantiation. When child components are added into parent components later, they must share the same version, according to the XML namespace that was assigned at instantiation. However, changing the _currentVersion after a component has been instantiated will not harm or change that component.
Thread Safe Approach: Each DDMS component would need to accept a current version variable in its data-based constructors rather than checking the singleton.

4) Using Component Builders ("top-down")
Current Impact: Each builder class relies upon the _currentVersion singleton at the moment that commit() is called to create an immutable component from the mutable builder. Generally, commit would be called one time at the Resource level, and convert the entire tree of child components at the same time. Changing the _currentVersion after a builder has been committed will not harm or change that component.
Thread Safe Approach: Each builder class would need to accept a current version variable as a set() accessor on the builder rather than checking the singleton.

Off the top of my head, a potential workaround that might satisfy your requirements with the existing library code would be this:

1) Employ Component Builders to do the transformations.
2) Minimize dependent code to call commit() just once per metacard, at the Resource.Builder level.
3) Set up a thread monitor / lock around the two lines of code that (a) set the DDMS Version and (b) commit the metacard.

If the above workaround is insufficient, please provide me with additional detail about your workflow and I'll see what I can come up with. If there are details that cannot be publicly shared, you can reach me privately at ddmsence AT urizone.net. I admit that I'm not a multithreaded expert, but would be willing to work with you to satisfy your specific case. I would also be willing to make changes to the library itself too, provided that I can balance a multithreaded solution with the intuitiveness of the existing single-threaded solution in a single new library version. (I would prefer to have a single latest release that handles all cases, rather than maintain separate multithreaded vs. single-threaded release lines).

Happy Thanksgiving,
BU

[Brendan Hofmann]

Brian,
Thank you for responding so quickly. I haven't had the chance to do an exhaustive analysis, but I have discovered a few things based on your feedback.

1) DDMSReader
I had hoped that this was true, but at least as of v2.3.0 that does not appear to be the case. Although the reader is instantiated with a specific version, it also calls setCurrentVersion() in buildResource. Since this alters the global state, it means that reading from a DDMSReader is not thread-safe. It is not immediately obvious to me why this should be the case, since as you noted in 2), the XML-based constructors should not use the singleton DDMSVersion.

3), 4)
As you noted, these rely on the singleton at various points. Making them thread-safe would require API changes. A possible solution would be to add a configuration property that controls whether components use the singleton or require a current version variable as part of their data model. This would preserve backwards compatibility while also permitting the implementation of a more thread-safe design.

Unfortunately, this issue cropped up relatively close to a release. Our customers care a lot about performance, so locking/synchronization is not going to be acceptable to them, since that pattern will inevitably slow down parallel usage. We have a workaround that will work for our implementation for now, but long-term improving DDMSence would probably be more optimal. If you are ok with introducing some optional verbosity and complexity for those who need it, while preserving the intuitive single-threaded design as the default and for backwards compatibility, then I don't think it would be too infeasible to satisfy both use-cases.

[Brian Uri!]

Brendan,

This is good food for thought -- thank you for the additional details. I have opened up Issue #222 to track work on this issue. I can get started on the research and design in the next two weeks, and will tentatively aim for a new release in January 2015.

I can't guarantee a release date for the new functionality, so hopefully your workaround will satisfy you for "a little bit longer". I can keep you posted if I expect any delay. Also, if your current workaround might help to inform my work, feel free to share, either here or in the Issue ticket.

Regards,
BU

[Brendan Hofmann]

I'd be happy to add a more detailed description of the proposed solution to issue #222 when I get the chance.

I don't think our workaround would be particularly helpful to you, since it doesn't involve modifying DDMSence at all. It'll work for now, but I don't want to encourage anyone into thinking that what I'm doing is a good idea in general. ;) Improving DDMSence still seems like the correct solution, it's just out of scope at this point in the development cycle.

Thanks again,
BH

[Brian Uri!]

For anyone following this thread and the related work,

I have released an unstable build of DDMSence with thread safety and attached it to the Issue ticket:
https://code.google.com/p/ddmsence/issues/detail?id=222

The JAR file and a diff of the changes can be found in comment #16:
https://code.google.com/p/ddmsence/issues/detail?id=222#c16

A draft of the documentation of changes that will end up on the DDMSence website can be found in comment #15:
https://code.google.com/p/ddmsence/issues/detail?id=222#c15

Because multithreading issues are more likely to arise in real world contexts than in my unit tests, I'm going to leave the unstable JAR file up for the remainder of this year so interested folks can try it out. I would appreciate any feedback or cursory tests if you expect to need these changes. Unless there are major concerns, I'll be aiming for a formal, stable release in the first few days of January 2015. Any issues that crop up after that time will, of course, be addressed in a rapid patch release (a matter of days rather than weeks).

Regards,
BU