adhearsion / blather

XMPP/Jabber Library and DSL for Ruby written on EventMachine and Nokogiri.
adhearsion.com/blather
Other
556 stars 89 forks source link

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

Closed benlangfeld closed 8 years ago

benlangfeld commented 9 years ago

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

mstate commented 9 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 https://github.com/adhearsion/blather/issues/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 commented 8 years ago

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

benlangfeld commented 8 years ago

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

chewi commented 6 years ago

@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 commented 6 years ago

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

chewi commented 6 years ago

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@e51bb1d1e7f271174244925cf8f431823979a74d. Reverting this against master fixes the issue, though there are still a handful of other failures. I'll see what I can do.

chewi commented 6 years ago

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 commented 6 years ago

Awesome work James! Thanks for the update.