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

SI-4520 - reasonable exception instead of OOE in XML parser #32

Closed
wants to merge 1 commit into from

Conversation

terma
Copy link
Contributor

@terma terma commented Nov 11, 2014

That's fix OutOfMemoryException for case when we parse valid XML, however user code request not existent content or not correct content type. Now parser instead of consume all memory just raise exception that request info not present.

Jira: https://issues.scala-lang.org/browse/SI-4339 and https://issues.scala-lang.org/browse/SI-4520

@terma
Copy link
Contributor Author

terma commented Nov 11, 2014

Can't find reviewer exactly for XML lib in http://www.scala-lang.org/contribute/hacker-guide.html
So please review @phaller
Thx

@adriaanm adriaanm modified the milestone: 1.0.3 Dec 3, 2014
@adriaanm
Copy link
Contributor

adriaanm commented Dec 3, 2014

Hi, @terma! I'm very sorry about the long wait for a review here. Might I suggest pinging one of the other recent submitters of PRs to review your fix? I'm not at all familiar with scala-xml, unfortunately. To facilitate the review, please expand your commit message with the reasoning behind the fix.

Also, SI-4520 is a duplicate of https://issues.scala-lang.org/browse/SI-4339.

I'm hoping to cut a 1.0.3 release soon and would like to include your fix.

@adriaanm
Copy link
Contributor

adriaanm commented Dec 3, 2014

(If not, there's always 1.0.4 :-))

@terma
Copy link
Contributor Author

terma commented Dec 4, 2014

@adriaanm thx for feedback. Not clear for me what reason I need to provide as that's simple bug not feature so I just fixed it. Any way I added more info to pr description

@som-snytt please review

@adriaanm
Copy link
Contributor

adriaanm commented Dec 4, 2014

Thank you! Could you please rebase on current master? While you're at it, could you include the PR description in the commit message -- PR descriptions are not preserved in git history.

@terma
Copy link
Contributor Author

terma commented Dec 4, 2014

OK Now I got your point will do
On Dec 3, 2014 10:06 PM, "Adriaan Moors" notifications@github.com wrote:

Thank you! Could you please rebase on current master? While you're at it,
could you include the PR description in the commit message -- PR
descriptions are not preserved in git history.


Reply to this email directly or view it on GitHub
#32 (comment).

@som-snytt
Copy link
Contributor

The Mark Harrah patch addresses the change at

scala/scala@0536364#diff-8561ae5555eec4758d457614adc309bc

by putting the exception in xToken.

Not every syntax error is fatal. For instance, pubidLiteral complains about bad chars but it can still consume the quoted string and continue parsing.

So the patch is a little better: xToken says it better be what I ask for.

This fix doesn't address the loop in xTakeUntil that doesn't check eof. That seems a direct response.

@adriaanm adriaanm added this to the 1.0.4 milestone Dec 4, 2014
@SethTisue SethTisue removed this from the 1.0.4 milestone Feb 4, 2017
@SethTisue
Copy link
Member

anyone interested in moving forward on this...?

@ashawley
Copy link
Member

Re-opened as #209 to see if these tests could be fixed, including some additional tests.

@ashawley ashawley closed this Apr 16, 2018
@ashawley ashawley mentioned this pull request Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants