Fix: Prevent resource leakage in SchemaFieldExtractor and FeedRepository#27449
Conversation
…dExtractor resource leaks. Fixes open-metadata#27062
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
| } catch (IOException e) { | ||
| LOG.error("Error closing schema input stream '{}': {}", schemaPath, e.getMessage()); | ||
| throw new SchemaProcessingException( | ||
| "Error loading schema '" + schemaPath + "': " + e.getMessage(), | ||
| "Error closing schema input stream: " + e.getMessage(), | ||
| SchemaProcessingException.ErrorType.INVALID_SCHEMA, | ||
| e); |
There was a problem hiding this comment.
💡 Edge Case: IOException catch misleadingly says 'closing' but covers all IO
In both loadMainSchema (line 194) and loadSchema (line 454), the outer catch (IOException e) block logs "Error closing schema input stream" but this catch will also fire if an IOException is thrown during getResourceAsStream() or while reading the stream (e.g., inside JSONTokener). The log message and exception text are misleading — they suggest only a close failure, when the real cause could be a read error.
Consider using a more general message like "Error reading schema input stream" to accurately reflect all possible IOException origins within the try-with-resources block.
Suggested fix:
Change the log and exception messages from:
"Error closing schema input stream ..."
to:
"Error reading schema input stream ..."
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsPrevents resource leakage in SchemaFieldExtractor and suppresses unnecessary logging in FeedRepository. Refactor the misleading 'closing' log message in the IOException catch blocks to accurately reflect potential read or stream retrieval errors. 💡 Edge Case: IOException catch misleadingly says 'closing' but covers all IO📄 openmetadata-service/src/main/java/org/openmetadata/service/util/SchemaFieldExtractor.java:194-199 📄 openmetadata-service/src/main/java/org/openmetadata/service/util/SchemaFieldExtractor.java:454-458 In both Consider using a more general message like "Error reading schema input stream" to accurately reflect all possible Suggested fix🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
There was a problem hiding this comment.
Pull request overview
This PR addresses backend debuggability and resource-handling issues by adding diagnostic logging during feed thread cleanup and ensuring schema resource streams are closed when loading JSON schemas.
Changes:
- Add WARN logging when
FeedRepository.deleteByAbout()fails to delete an individual thread. - Wrap schema resource
InputStreams in try-with-resources inSchemaFieldExtractorto prevent resource leakage. - Add explicit handling for stream-close failures during schema loading.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/util/SchemaFieldExtractor.java | Uses try-with-resources for schema InputStreams and adds close-failure handling (but currently introduces compilation issues that must be fixed). |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/FeedRepository.java | Logs thread deletion failures instead of silently swallowing exceptions. |
| } catch (Exception e) { | ||
| LOG.error("Error loading schema '{}': {}", schemaPath, e.getMessage()); | ||
| throw new SchemaProcessingException( | ||
| "Error loading schema '" + schemaPath + "': " + e.getMessage(), | ||
| SchemaProcessingException.ErrorType.INVALID_SCHEMA, | ||
| e); | ||
| } | ||
| } catch (IOException e) { | ||
| LOG.error("Error closing schema input stream '{}': {}", schemaPath, e.getMessage()); | ||
| throw new SchemaProcessingException( | ||
| "Error loading schema '" + schemaPath + "': " + e.getMessage(), | ||
| "Error closing schema input stream: " + e.getMessage(), | ||
| SchemaProcessingException.ErrorType.INVALID_SCHEMA, | ||
| e); |
| SchemaProcessingException.ErrorType.OTHER); | ||
| } | ||
| } |
| } catch (Exception e) { | ||
| LOG.error("Error loading schema '{}': {}", schemaPath, e.getMessage()); | ||
| throw new SchemaProcessingException( | ||
| "Error loading schema '" + schemaPath + "': " + e.getMessage(), | ||
| SchemaProcessingException.ErrorType.OTHER); | ||
| } | ||
| } catch (IOException e) { | ||
| LOG.error("Error closing schema input stream '{}': {}", schemaPath, e.getMessage()); | ||
| throw new SchemaProcessingException( | ||
| "Error loading schema '" + schemaPath + "': " + e.getMessage(), | ||
| "Error closing schema input stream: " + e.getMessage(), | ||
| SchemaProcessingException.ErrorType.OTHER); |
Resolves issue #27062. Surfaced a mistakenly swallowed generic Exception within FeedRepository thread deletions, and utilized try-with-resources internally for loadSchema and loadMainSchema inside SchemaFieldExtractor.java to close unused stream locks.