Avoid threading issues in sortFields (#1686)

[bendk]

I used essentially the same pattern than validateFields uses.

[bendk]

I tried adding tests for this, but I couldn't figure out a good way to reliable trigger the issue.

[matthiasblaesing]

I don't see the solution in sortFields. That method does exactly what it tells us. It also does not access global state, so there is no need for locking inside sortFields. The problem is that the list returned by getFieldList in line 1128 is modified via the call to sortFields in line 1161 and that list is shared.

jna/src/com/sun/jna/Structure.java

Lines 1127 to 1163 in 1d3ceb7

	protected List<Field> getFields(boolean force) {
	List<Field> flist = getFieldList();
	Set<String> names = new HashSet<>();
	for (Field f : flist) {
	names.add(f.getName());
	}
	List<String> fieldOrder = fieldOrder();
	if (fieldOrder.size() != flist.size() && flist.size() > 1) {
	if (force) {
	throw new Error("Structure.getFieldOrder() on " + getClass()
	+ (fieldOrder.size() < flist.size()
	? " does not provide enough"
	: " provides too many")
	+ " names [" + fieldOrder.size()
	+ "] ("
	+ sort(fieldOrder)
	+ ") to match declared fields [" + flist.size()
	+ "] ("
	+ sort(names)
	+ ")");
	}
	return null;
	}
	Set<String> orderedNames = new HashSet<>(fieldOrder);
	if (!orderedNames.equals(names)) {
	throw new Error("Structure.getFieldOrder() on " + getClass()
	+ " returns names ("
	+ sort(fieldOrder)
	+ ") which do not match declared field names ("
	+ sort(names) + ")");
	}
	sortFields(flist, fieldOrder);
	return flist;
	}

sortFields is only called by getFields and that is only called by deriveLayout and that is called by calculateSize. For "normal" structures caching kicks in, only structures containing arrays will not be cached. So from my POV the most obvious fix would be to only change the original line 1128 to:

    protected List<Field> getFields(boolean force) {
         // line 1128:
        List<Field> flist = new ArrayList<>(getFieldList());
        Set<String> names = new HashSet<>();

That way there is no global state and we need no extra layers.

For anything more complex I want to see numbers.

[matthiasblaesing]

And the important thing i forgot: thanks for taking care 👍

[brettwooldridge]

I think I agree with @matthiasblaesing. Ultimately, because the LayoutInfo is cached globally by calculateSize(), the result is deriveLayout() -> getFields() -> sortFields() is only called once for "normal" structures, and therefore there seems no need to cache the sorted fields in a separate global cache.

The only arguable reason to do this would be to speed-up calculating the size of "variable" Structures (LayoutInfo.variable), which are not cached, but the overhead of @matthiasblaesing suggestion of...

    protected List<Field> getFields(boolean force) {
         // line 1128:
        List<Field> flist = new ArrayList<>(getFieldList());
        Set<String> names = new HashSet<>();

...is miniscule in comparison to the work that deriveLayout() already has to do in that case, and therefore of almost no benefit.

At least that's my understanding at present.

[bendk]

@matthiasblaesing thanks for the suggestion, I'm very happy to go with the simplified version. I didn't understand the caching strategy/requirements when I added that extra code. I'll push a new version up tomorrow.

[bendk]

What do you think of making a 5.18.1 release for this?