Add endpoint parameter and fix resource leaks - #1
Draft
Watson1978 wants to merge 6 commits into
Draft
Conversation
Running embulk.run() in a loop inside a single JVM accumulated resources on every run and eventually led to an OutOfMemoryError. The S3Client built by S3ClientManager was never closed anywhere in the repository. Propagate close() from S3ClientManager through AbstractStrategy down to each strategy, so the SDK resources are released on Embulk's task completion lifecycle. embulk-core calls close() after commit() / abort(), so the client is no longer in use at that point. In FileOutputStrategy the super.close() call sits in a finally block so the client is released even when closeBuffer() throws. Also build the S3ClientManager after validate() rather than before it. When validate() throws, the instance never reaches FileOutputRunner#open, so Embulk never gets a chance to call close() and the client was leaked. The exception types and messages are unchanged. Also shut down the ExecutorService that multiPartUpload() creates on every call. It was never shut down, leaking non-daemon threads per upload. Move the getRegion() stub in BufferedStrategyTests from setUp() to testValidateTrue(): the validation-failure path no longer builds a client, so Mockito's strict stubs flagged the stub as unnecessary.
./gradlew build was green while running zero tests. The dynamic version
'org.junit.jupiter:junit-jupiter:5.+' resolved to 5.14.4 (junit-platform
1.14.4), while Gradle 5.2.1 bundles junit-platform-launcher 1.3.1. The
generation gap made test discovery fail, and because that failure is only a
warning, the build stayed green:
TestEngine with ID 'junit-jupiter' failed to discover tests
org.junit.platform.commons.JUnitException: OutputDirectoryCreator not
available; probably due to unaligned versions of the junit-platform-engine
and junit-platform-launcher jars
A recent change broke an existing test and the build did not notice.
Pin junit-jupiter to 5.14.4 and put a matching junit-platform-launcher 1.14.4
on the test runtime classpath, which Gradle 5.2.1 prefers over its bundled
one. All 10 tests now run, and a deliberately broken assertion does fail the
build.
Pin mockito to 3.12.4 (the version '3.+' resolved to) for the same reason: a
dynamic version can break the build without anything changing here.
Drop jcenter(), which shut down in 2022. Every dependency resolves from
mavenCentral() alone, verified with --refresh-dependencies.
An application that embeds the plugin via EmbulkEmbed and loops embulk.run()
over many tables grows its heap on every run until it dies with an
OutOfMemoryError.
The caller hands the ownership of the buffer over to add(). embulk-core's
FileOutputOutputStream#doFlush() calls out.add(buffer) and then drops its own
reference with buffer = BufferImpl.EMPTY -- it never releases it. The canonical
implementation, OutputStreamFileOutput#add() in embulk-core, therefore releases
the buffer in a finally block. This plugin did not, so every pooled buffer it
was handed stayed allocated in the Netty heap arena, and the arena never
reclaims it: EmbulkEmbed builds one BufferAllocator at construction time and
shares it across every run. Each run leaked as many bytes as it wrote.
Measured with the real FileOutputStrategy driven through the real
FileOutputOutputStream, one shared allocator, 32MB per run (heap after GC):
before after OutputStreamFileOutput (control)
after run 1 45 MB 13 MB 7 MB
after run 2 77 MB 13 MB 7 MB
after run 4 141 MB 13 MB 7 MB
after run 8 269 MB 13 MB 7 MB
Releasing after the synchronous write to the local file is safe: nothing keeps
a reference to the buffer's array past add().
Known and NOT fixed here: BufferedStrategy#add() has the same missing release,
but it retains the pooled array via ByteBuffer.wrap() until commit(), so simply
releasing there would corrupt data once the array is recycled. Fixing it needs
a copy-and-accumulate change, which belongs with the separate bug where that
strategy silently drops everything but the last buffer. It is a non-default path
(enable_temp_file_output: false) and does not match the reported symptom.
Watson1978
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.