On 6/26/2020 12:46 PM, Stuart Marks wrote:
On 6/25/20 4:53 PM, Joe Wang wrote:
Please review a fix to a BCEL regression. At issue was the addition
of hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
method was implemented to return the instruction's opcode that
unfortunately is mutable. The problem hasn't showed up until the code
path led to calls to remove from a HashSet a target that has been
changed since it was added to the HashSet. The proposed patch is to
make the hashCode final/immutable.
This patch implies that a target object is considered the same one
even if its field values may have been changed. It therefore may not
be appropriate in other situations (or may cause problems). However,
since it had always had no hashCode() override before BCEL 6.0,
thereby relying on Object's identity hash code, its use case has been
limited and time-tested. It can benefit from this patch in that it
provides the same function as Object's hash code, and then serves as
a reminder (to any who might read the code) how it was used (objects
considered to be the same over the course as far as the hashCode is
concerned).
JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/
Hi Joe,
This fix seems vaguely wrong, but in the context of what I understand
it's doing, it seems that BCEL or the surrounding JAXP code is even
more wrong. :-)
The patch is surely unusual, because of this unusual case. As far as
which one is more wrong, I believe the burden is mostly on BCEL although
JAXP might share a bit, at the least it exposed the issue.
From what I understand, this bugfix was prompted by a case where an
Instruction was added to a HashSet. The Instruction was then mutated
while in the HashSet, and this resulted in it not being removed from
the HashSet in the future, when it was necessary to do so, thus
leaving a stale entry. How could this have possibly worked?!?
It works in most majority of the cases, although I think the BCEL
developers admitted they had limited tests. This is because while
Instruction allows mutation, all but one type of instructions actually
do, and the mutated one don't get removed from the HashSet in a normal
build-up process. But XML applications can go berserk sometimes, and in
this case resulted in a method exceeding the max method size. When that
happens, a rebuild process involved many copies/clones, resetting
targeters, the later included removing targeters (among them the mutated
ones) from the HashSet. Given that such a big stylesheet is unusual, it
makes it an edge case. At least, we haven't received any error reports
from the 20% who adopted Java 11.
I'm speculating a bit, but I could imagine code like this:
var inst = new Instruction(ORIG_OPCODE);
var set = new HashSet<Instruction>();
set.add(inst);
...
inst.setOpcode(NEW_OPCODE); // (1) mutates inst while in HashSet!
...
set.remove(inst); // (2)
In the current version, the hashCode() is simply the opcode, and
equals() compares opcodes (and possibly other things). This is correct
from the standpoint of Instruction itself, as equal items have the
same hashcode. However, the above code fails because at line (2), the
remove() method looks in the "wrong" hash bucket and can't find the
instruction, so it does nothing.
Yes, it will be correct for the majority cases where opcodes don't
change. Also note that HashSet checks hashcode, and then the key's
identity before key.equals, which is why this patch works. When the
build process calls on an instruction to dispose itself, it removes
itself from the targeter HashSet. But while it is itself, if the
hashcode has changed, it won't even find itself! (sort of lost itself)
(that's too many "itself")
The patch changes the class to compute the hashcode at construction
time, and makes the hashcode constant, so it "fixes" the above code,
since the remove() method will look in the same bucket and find the
desired instance. But now Instruction breaks the hashCode/equals
contract, since it's possible for two instances to be equal but to
have different hashcodes. (Consider an Instruction that was created
with NEW_OPCODE. It can be equal to inst, but its hashcode will differ.)
It would break the hashCode/equals contract if it was implemented
normally. But this is unusual, the equals implementation was nothing but
normal: for branch instructions, it always returns false! Thus, this
unusual hashCode implementation will not break the contract.
It seems to me that this patch is compensating for broken code
elsewhere in BCEL or JAXP by adding more brokenness. It could be that
is ok in this narrow usage context. That is, as far as we know, this
"shaded" copy of BCEL is used *only* by JAXP and not used in general,
and it may be that the particular ways that JAXP uses BCEL aren't
broken (and are indeed fixed) by this change. But I'd like to see some
assurance of that. It could be that there are bugs introduced by this
change that simply haven't been uncovered by the testing we've done yet.
I mentioned above, I believe the burden was mostly on BCEL. JAXP/Xalan
might have complicated things, the way it rebuilds the instruction list
when it encounters an over-the-limit method, or it could have been
smarter to avoid building such a large list. But it was all done through
BCEL's public APIs. Nothing JAXP/Xalan has done was not within the
functions BCEL provided.
In BCEL, not only the equals were implemented with some special
treatment for certain subtypes, the choose of HashSet to store mutable
(instruction) objects may arguably not have been the best.
Another approach is to figure out what is mutating the Instruction
while it's in a HashSet and fix that instead.
Re-setting Instruction target triggered it. But unfortunately that's
necessary as far as the current process goes.
Unfortunately, either of these approaches seems to involve an
arbitrary amount of work. :-(
Very true, and that's why I provided this shaky yet
working-for-the-hacky-code patch, it's a patch on patch rather than
something one could do properly when designing a new code.
That's not to say the BCEL engineers hadn't put in a great effort.
They've worked on it for about half a year related to this
hashCode/Equals issue in BCEL 6.0. They also deliberated replacing
HashSet. But I guess eventually reality sets in, it would have been a
major architectural work.
All that's said, while it has its (maybe major) problems as this issue
revealed, BCEL has been a stable component for JAXP/java.xml. This issue
introduced by the changes in BCEL 6.0, I believe, is an edge case as
explained previously. If the instruction list is built
straight-forwardly, it would not have run into this issue. This patch
resolves the case, restores the delicate balance and hopefully keep BCEL
as stable as before.
-Joe
s'marks
