Skip to content

[ISSUE #3009] Method manually handles closing an auto-closeable resource [MetricsHandler] #3547

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

Closed
wants to merge 5 commits into from

Conversation

sanskar-thakur
Copy link
Contributor

@sanskar-thakur sanskar-thakur commented Mar 28, 2023

Fixes #3009 .

Modifications

Replaced finally block with try-with-resources.

  1. Used try-with-resources along with nested try-catch to manage resources, fixing out of scope variable.
  2. Added similar fix for issue#3007 and issue#3008 to fix compilation error due to out of scope variable in catch block.

Please include this fix to solve compilation issue occurred due to #3007 and #3008

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)
  • If a feature is not applicable for documentation, explain why? (bug fix)
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation (not applicable)

Alonexc
Alonexc previously approved these changes Mar 29, 2023
Copy link
Contributor

@Alonexc Alonexc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@mytang0 mytang0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Why?

@Alonexc
Copy link
Contributor

Alonexc commented Mar 29, 2023

Here you may still have to use try-with-finally, using try-with-resources will be due to the scope of the variable, here in the catch there is out.write(result.getBytes()); this line of code is reported as an error. Do you have a better suggestion?

1. Used try-with-resources along with nested try-catch to manage resources, fixing out of scope variable.
2. Added similar fix for issue#3007 and issue#3008 to fix compilation error due to out of scope variable in catch block.
@mytang0
Copy link
Member

mytang0 commented Mar 30, 2023

Linked to #3544, PR overlapped. @sanskar-thakur

@xwm1992 xwm1992 changed the title [Enhancement] Method manually handles closing an auto-closeable resource [MetricsHandler] #3009 [ISSUE #3009] Method manually handles closing an auto-closeable resource [MetricsHandler] Mar 30, 2023
Zaar added 3 commits April 1, 2023 20:53
Used try-with-resources along with nested try-catch to manage resources, fixing out of scope variable.
1. Removed nested try-catch and used single try-with-resources.
2. For out of scope out variable, replaced with log result.
@scwlkq
Copy link
Contributor

scwlkq commented Jan 20, 2024

@Pil0tXia this pr seems to be merged after the second reviewer reviews

@Pil0tXia
Copy link
Member

Please resolve conflicts.

@pandaapo pandaapo closed this Mar 22, 2024
@pandaapo
Copy link
Member

completed in #4800

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.

[Enhancement] Method manually handles closing an auto-closeable resource [MetricsHandler]
7 participants