-
-
Notifications
You must be signed in to change notification settings - Fork 897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JRuby cache clearing issue since 1.6.2 affects Blather (Adhearsion) #1708
Comments
It looks like the |
Clearing cached nodes was necessary under certain circumstances because the cached instance of the node was bound to the wrong namespace. I believe I also added tests to guard against regression, so if the tests still pass without the That said, I haven't done any work on nokogiri's Java internals since late 2014, so basing anything on my current memory and knowledge is exactly as risky as you might think. 😄 |
Thanks for letting us know. I can go back to that commit and check whether removing that line breaks your test when I get a minute. |
I definitely checked and it doesn't break any tests. Given my initial
confusion (hence the TODO in the code) and the fact that it doesn't
cause any tests to break atm. I am inclined to get rid of that line.
|
I can confirm that going back to e51bb1d and removing both |
I'm not sure I understand what's being suggested for next steps. I'd be happy to merge a PR ... |
It was previously described at "weird" and even the original author cannot remember why it is there. Its presence causes Blather's Blather::XMPPNode subclass instances to be recreated where they become generic Nokogiri::XML::Element instances instead. Closes sparklemotion#1708.
Opened a pr #1716 with the proposed changes
|
Hmm, I already opened #1715. I only removed the first call though so you'll need to decide which one to go with. |
I think it is fine to get rid of both calls since neither one affect the tests
|
Okay, closed mine. |
The Blather project, an important part of Adhearsion, has been severely affected by a cache clearing issue in the JRuby implementation since 1.6.2, and as such has had to lock the Nokogiri version back to 1.6.1. This is obviously a security concern and we are missing out on other fixes made since.
Blather (indirectly) subclasses
Nokogiri::XML::Node
asBlather::XMPPNode
. It's hard to explain exactly what happens as I'm not the author and it's not the simplest of libraries but somewhere along the line,node.parent
is called and rather than returning the originalBlather::XMPPNode
instance that was created earlier, a newNokogiri::XML::Element
instance is returned instead. It may not matter that the instance is different but the change in class certainly does matter. It should go without saying that this does not happen under MRI.I bisected the problem back to e51bb1d, made by @mbklein. I obviously had to raise an eyebrow when I saw that the following comment had since been added above the first clearCachedNote statement by @jvshahid.
Indeed, if I comment out this line, the problem goes away and all the Nokogiri tests still pass. Unhelpfully, if I comment out the other clearCachedNode line below, all the Nokogiri tests still pass, so perhaps there is insufficient testing here.
My limited understanding tells me that because the cache is being cleared, the original Ruby instance is lost and a new one is being created in
NokogiriHelpers#constructNode
with the hardcoded class ofNokogiri::XML::Element
. Given that we only have theorg.w3c.dom.Node
instance to recreate from, Ruby-specific details such as which subclass was used originally, are lost.It is not clear to me exactly when the cache is supposed to be cleared. If it can happen quite easily then perhaps the approach that Blather takes is no longer viable. However, given the interesting comment noted above, I suspect it is currently being cleared more than it should.
The text was updated successfully, but these errors were encountered: