[GitHub] [groovy] smagill opened a new pull request #1393: Resource leak fix (#2)

[GitBox <gi...@apache.org>]

smagill opened a new pull request #1393:
URL: https://github.com/apache/groovy/pull/1393


   Fix a resource leak warning.  While CharBuf.close is currently a no-op, CharBuf does inherit from Writer, which has the contract that it should be closed after use.  This protects against the case where CharBuf is later changed to involve resources that should be freed.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [groovy] paulk-asert commented on pull request #1393: Resource leak fix (#2)

[GitBox <gi...@apache.org>]

paulk-asert commented on pull request #1393:
URL: https://github.com/apache/groovy/pull/1393#issuecomment-814560676


   I merged the .muse config settings and added the comment around the currently false positive warning. Thanks!


-- 
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:
users@infra.apache.org

[GitHub] [groovy] paulk-asert closed pull request #1393: Resource leak fix (#2)

[GitBox <gi...@apache.org>]

paulk-asert closed pull request #1393:
URL: https://github.com/apache/groovy/pull/1393


   


-- 
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:
users@infra.apache.org

[GitHub] [groovy] paulk-asert commented on pull request #1393: Resource leak fix (#2)

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org