Skip to content
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

Stream negotiation doesn't work with Nokogiri >= 1.6.2 #145

Closed
benlangfeld opened this issue Dec 15, 2014 · 10 comments
Closed

Stream negotiation doesn't work with Nokogiri >= 1.6.2 #145

benlangfeld opened this issue Dec 15, 2014 · 10 comments
Labels

Comments

@benlangfeld
Copy link
Member

Features apparently don't get parsed correctly. This may only be true on JRuby.

@mstate
Copy link

mstate commented Nov 7, 2015

I think I've isolated the issue on JRuby. It seems as though Blather::Stream::Parser:70

       @current = @current.parent

intends to return a Blather::XMPPNode but instead returns Nokogiri::XML::Element. When to_stanza is called on the Nokogiri::XML::Element at Blather::Stream:232

 @receiver.receive_data node.to_stanza

stream throws an exception as the to_stanza method doesn't exist for Nokogiri::XML::Element.

On MRI ruby, the parser returns a Blather::XMPPNode and all is well.

I've been trying to figure out how to solve it but still haven't got it. Probably a bug is required on Nokogiri and perhaps a workaround on Blather.

@bklang
Copy link
Member

bklang commented Nov 10, 2015

@mstate Thank you very much for the additional help in tracking this down. Ben Langfeld is on vacation this week and I'm traveling so neither of us have had time to look at it. I just wanted to let you know that we'll get back to this as soon as possible.

benlangfeld added a commit that referenced this issue Jan 6, 2016
I wear I will never use Nokogiri ever again

#145
@benlangfeld
Copy link
Member Author

Thanks for taking a look at this, @mstate.

"Fixed" by refusing to support Nokogiri > 1.6.1. Nokogiri on JRuby is someone's very shitty idea of a joke; it has caused me a lot of pain over the last few years contributing to and maintaining Blather and I am now at the point of outright refusing to deal with any more of its' bullshit.

If someone wants to get our test suite passing on JRuby, I will very gladly merge those changes, but short of completing #71 I do not believe it to be possible, and that would constitute a major version bump (which is not a bad thing). I personally have no energy for doing this nights & weekends.

Call it a known problem in search of someone who cares enough to do something about it.

@singpolyma
Copy link
Contributor

Since this seems limited to JRuby, could not newer Nokogiri versions be supported for MRI?

@benlangfeld
Copy link
Member Author

@singpolyma Possibly. If you have the inclination to prep such a proposal and test it, I'd consider merging it :)

@chewi
Copy link
Contributor

chewi commented Dec 19, 2017

@benlangfeld, I'm going to brave this one, at least for a quick look. Was the issue ever reported upstream? I did look but you've commented on so many issues!

@benlangfeld
Copy link
Member Author

I don’t think it was reported upstream, no.

@chewi
Copy link
Contributor

chewi commented Dec 21, 2017

I have a little good news. I bisected this between 1.6.1 and 1.6.2 and found the breaking commit to be sparklemotion/nokogiri@e51bb1d. Reverting this against master fixes the issue, though there are still a handful of other failures. I'll see what I can do.

@chewi
Copy link
Contributor

chewi commented Jan 15, 2018

More good news. Upstream agrees that the offending line is suspicious, in fact they'd already commented on it years ago. They haven't done it yet but it will probably be dropped. A couple of failures turned out to be pendings that used to fail under JRuby but no longer do. One turned out to be a strange string pointer bug in JRuby-OpenSSL that's already been fixed. A whole bunch were due to a Nokogiri bug that created useless blank text nodes and that's already been fixed. There are 15 failures remaining and they all carry the same error message. No idea what it means yet but hopefully I can figure it out.

@bklang
Copy link
Member

bklang commented Jan 15, 2018

Awesome work James! Thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants