paulk-asert commented on pull request #1393: URL: https://github.com/apache/groovy/pull/1393#issuecomment-704240031
Nice detective work. I have two concerns though. Firstly, I think this is very performance sensitive code in terms of JSON parsing. The wrapping with try might totally bog down parsing. I don't know for sure though and we'd need to run a little benchmark analysis to better guide our decisions. Secondly, returning `null` isn't likely to play nicely with existing code that users this. I think we'd just fail fast and wrap the IOException in a RuntimeException and throw that. As you already spotted, CharBuf.close is currently a no-op, and the impacted classes aren't likely to be changed much, so all the above seems like a bit of an overkill reaction to the issue. My recommendation would be just to add a comment saying something like "consider wrapping in a try with resources block if CharBuf is ever refactored to have a non-empty close()". We could also provide an appropriate hush for the resource leak warning. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
