parseString callback never called #51

Closed
frederic-h opened this Issue Jun 7, 2012 · 22 comments

Projects

None yet

8 participants

@frederic-h

I am using xml2js to parse XML-data from an external source I do not control.

I recently encountered a piece of invalid XML that caused parseString to not call the callback, ever. Not with a result, not even with an error.

Here's a sample node app and 2 XML-files that cause the described behavior:

test.js:

var fs = require( "fs" ), xml2js = require( "xml2js" );

var parser = new xml2js.Parser();
fs.readFile( __dirname + "/foo.xml", function( err, data ) {
    if( err ) {
        console.log( "file io error: " + err );
        return;
    }
    parser.parseString( data, function( err, res ) {
        // never reached
        if( err ) {
            console.log( "parse error: " + err );
            return;
        }
        console.dir( res );
        console.log( "done" );
    } );
} );

foo.xml:

<?xml version="1.0"?>

foo2.xml

<?xml version="1.0"?>
This is a string.
This is another string.
@Leonidas-from-XIV

Reminds me of bug-report #3. I'll look into it but most likely this is caused by a bug in sax-js.

@c0bra
c0bra commented Sep 25, 2012

I'm having this issue as well with invalid xml.

@Leonidas-from-XIV

Bugs with invalid XML are most likely a bug in sax-js, resulting in error callback being never called.

@jlfaber
jlfaber commented May 6, 2013

I ran into a similar issue with xml input that omitted the close element for the root tag. It looks like there are two underlying concerns. First, as noted above, node-sax version 0.4.2 and 0.4.3 do not properly return errors in this situation. Starting with version 0.5.0 that appears to have been remedied. So, for starters, a bump to 0.5.0 (minimum) would be helpful.

In addition, I believe xml2js itself should be calling the close method on the underlying sax parser object immediately after calling write with the input string. This allows the parser to detect the unclosed root tag and return an error. I've tried all three cases (the two above and my malformed xml) and all three are properly handled after making those changes.

@Leonidas-from-XIV
Owner

That is a good point. Waht I'll do is release the current code with the current version that is tested and then bump to 0.5.0 on GitHub so there will be some time to determine whether there are regressions.

@Leonidas-from-XIV
Owner

@jlfaber Could you provide some testcases that are fixed when using the close method, so I can add a regression test?

@jlfaber
jlfaber commented May 6, 2013

With 0.5.0, I believe the two listed above (foo.xml and foo2.xml) should both be fixed (i.e. return errors), as should the following simple case:

foo3.xml

<?xml version="1.0"?>
<unclosedRootTag>

Not sure what just adding the close() without bumping 0.5.0 is going to do. Guess that's what testing is for...

Also, I scanned the delta between 0.5.0 and 0.5.4 (current) and all the changes look pretty benign. I'd suggest using a more permissive dependency specification like "0.5.x" rather than locking into a specific version, as recommended here: http://blog.nodejitsu.com/package-dependencies-done-right. I realize there's some level of trust required to do that (specifically that the dependent module author will respect semver and not introduce incompatible changes), but it certainly makes it easier to pick up bug fixes. Thanks for the quick response, btw!

@Leonidas-from-XIV
Owner

I'll add the close() later, I broke my finger so lately maintenance has been quite slow.

I used to have something like >= x.y.z but that didn't work so well, I even got a bug report, #68. The problem was that I often got bug reports that I just couldn't reproduce because they were (maybe) using a broken version of sax-js, so I pinned it to the version that I have tested it. I usually bump the versions before releasing.

So, it is kinda difficult. Maybe sax-js got more mature so I could try using a 0.5.x restriction, I should think about it some more.

@jlfaber
jlfaber commented May 6, 2013

I agree that ">= x.y.z" is problematic since it allows both the major and minor versions to increase as well as the patch version. If the sax author is properly following the semver.org rules, though, allowing "0.5.x" should be safe since only the patch version is allowed to increase. And by contract, that means that the public API has not changed.

I understand the danger of bugs in future versions of sax But that has to be balanced against the danger of undiscovered bugs in the current version of sax. When that happens (and there are ALWAYS undiscovered bugs), if the sax version is pinned, I need not only to get the bug fixed in sax, but I have to trouble you to update your module as well to pick up that change. For any component with more than a couple of dependencies, that will quickly turn into a mess! Specifying a particular major and minor version, but allowing patch to float is much more scalable IMO.

@Leonidas-from-XIV
Owner

Is there a way to specify "0.5.x where x > 3" or something?

@jlfaber
jlfaber commented May 6, 2013

I think you can use a tilde version spec for that. "~0.5.4" is technically equivalent to ">=0.5.4 <0.6.0". See the docs here: https://npmjs.org/doc/json.html

@guillermosan

I'm having this issue too. The close() after the write in parseString solves it.

@Leonidas-from-XIV

@guillermosan Can you post a simple XML file that fails without the close() and succeeds with it? I could add a regression test for it. When I just add close() the testsuite basically explodes, I need to look into it what's wrong.

@guillermosan

@Leonidas-from-XIV There you go, but no new info. It's the same @jlfaber posted.

<?xml version="1.0"?>
<unclosedRootTag>

Without close() this will make parseString to never call the callback. With the close() it will call the cb with error "Unclosed root tag".

@Leonidas-from-XIV

Alright. Unfortunately, it also crashes a lot of unit tests which use well-formed XML.

@Leonidas-from-XIV Leonidas-from-XIV added a commit that closed this issue Jan 2, 2014
@Leonidas-from-XIV Merge branch 'tsgautier-master'
Also added tsgautier to contributors.

Closes #113
Closes #51
ad05a02
@voronianski

@Leonidas-from-XIV I'm having that issue right now with version 0.4.6 that is latest:

[Error: Unclosed root tag
Line: 0
Column: 15
Char: ]

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: Unclosed root tag
Line: 0
Column: 15
Char:
    at error (/Users/dmitri/trinity-mirror/football-modular/football-betting-api/node_modules/xml2js/node_modules/sax/lib/sax.js:642:8)
    at strictFail (/Users/dmitri/trinity-mirror/football-modular/football-betting-api/node_modules/xml2js/node_modules/sax/lib/sax.js:662:22)
    at end (/Users/dmitri/trinity-mirror/football-modular/football-betting-api/node_modules/xml2js/node_modules/sax/lib/sax.js:649:27)
    at Object.write (/Users/dmitri/trinity-mirror/football-modular/football-betting-api/node_modules/xml2js/node_modules/sax/lib/sax.js:916:30)
    at Object.SAXParser.close (/Users/dmitri/trinity-mirror/football-modular/football-betting-api/node_modules/xml2js/node_modules/sax/lib/sax.js:152:38)
    at Parser.exports.Parser.Parser.processAsync (/Users/dmitri/trinity-mirror/football-modular/football-betting-api/node_modules/xml2js/lib/xml2js.js:249:31)
    at Object._onImmediate (/Users/dmitri/trinity-mirror/football-modular/football-betting-api/node_modules/xml2js/lib/xml2js.js:6:59)
    at processImmediate [as _immediateCallback] (timers.js:354:15)

That's very annoying 'cause it crashes the app and I have no chance to intercept it in callback of parseString.

@Leonidas-from-XIV
Owner

Can you post a simple XML to reproduce?

@voronianski

@Leonidas-from-XIV just try '<xml>POST VALUE'.

@voronianski

@Leonidas-from-XIV I know that xml is extremely invalid but I'm working with resource that can send crazy responses..

@saurabhtaneja

You can take the xml in blocks and if find any new line then add into the block if index goes to - then console you will find full xml then use it like
var callbackres = callbackresxml.toString('utf8');
callbackblock += callbackres;
if(callbackres.indexOf("\n")> -1){console.log(callbackblock );}

@raeesaa
raeesaa commented May 18, 2015

Even I am facing 'Unclosed Root Tag' error for valid XML. The version of library I am using is 0.4.4. Was anyone able to find workaround for this?


events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: Unclosed root tag
Line: 1098
Column: 274
Char: 
    at error (/home/user/user/Test/node_modules/xml2js/node_modules/sax/lib/sax.js:642:8)
    at strictFail (/home/user/user/Test/node_modules/xml2js/node_modules/sax/lib/sax.js:662:22)
    at end (/home/user/user/Test/node_modules/xml2js/node_modules/sax/lib/sax.js:649:27)
    at Object.write (/home/user/user/Test/node_modules/xml2js/node_modules/sax/lib/sax.js:916:30)
    at Object.SAXParser.close (/home/user/user/Test/node_modules/xml2js/node_modules/sax/lib/sax.js:152:38)
    at Parser.exports.Parser.Parser.parseString (/home/user/user/Test/node_modules/xml2js/lib/xml2js.js:403:67)
    at Parser.parseString (/home/user/user/Test/node_modules/xml2js/lib/xml2js.js:6:61)
    at Socket.<anonymous> (/home/user/user/Test/ftpfileSaveWorker.js:43:20)
    at Socket.emit [as _emit] (events.js:95:17)
    at Socket.source.emit (/home/user/user/Test/node_modules/ftp/lib/connection.js:564:20)

@Leonidas-from-XIV
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment