Repository navigation
fix: v1.24.1: DBObjectDirectoryAdapter: limit concurrent object file reads - #187
Merged
Merged
Conversation
`selectAll` and `selectByIDs` read all their objects in parallel (an async `readAsString` per object file), so concurrent selects of a large table opened thousands of files at once: not a leak (all closed afterwards), but an unbounded burst. Measured with `lsof`: 3875 files in `bones_api_concurrency_memory_test` (`DBObjectDirectoryAdapter` group) and 24000 for 30 concurrent `selectAll`s of 1000 objects, failing with `Too many open files` (`errno = 24`) under the macOS default limit (256). - `DBObjectDirectoryAdapter`: object reads through a `FileLimited` semaphore (50): the peak above drops from 3875 to 38. - `FileLimited.readAsString`: like `File.readAsString`, under the semaphore. Tests: - `bones_api_entity_db_directory_test`: concurrent `selectAll`s in a process limited to 256 open files (`ulimit -n 256`); fails with `errno = 24` without the fix. - `FileLimited.readAsString`. - `bones_api_concurrency_memory_test` now passes with `ulimit -n 256`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #187 +/- ##
==========================================
- Coverage 95.29% 95.28% -0.01%
==========================================
Files 68 68
Lines 23771 23774 +3
==========================================
+ Hits 22653 22654 +1
- Misses 1118 1120 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- `_readObject`: no `existsSync` before the (queued) read: a file deleted while the read waits for the semaphore reads as `null` (it threw `PathNotFoundException`, failing the whole select). Also one blocking syscall less per row. - `_doDeleteImpl`: reads synchronously (not queued behind the reads of concurrent selects), and a concurrent delete of the same object deletes nothing instead of throwing. - Selects read in batches of the `fileLimited` limit: concurrent selects queue at most a batch of reads each, not one per object. - `DBObjectDirectoryAdapter.fileLimited`: the shared semaphore, exposed to tune it (`FileLimited` also exported by `bones_api_db_directory.dart`). - `FileLimited`: the acquire/release lives in one `_run`, releasing the semaphore it acquired: an operation in flight while `setSemaphoreLimit` replaced the semaphore released the new one (above its limit). - Regression test: package root resolved (any working directory), the script run directly (`dart --packages`, not `dart run`: no package resolution or build hooks under the low limit), skipped if the limit can't be set. - Tests: concurrent deletes + selects, `fileLimited` batches, semaphore swap with operations in flight. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Summary
bones_api_concurrency_memory_test.dartfailed under the macOS default limit of open files (256) withToo many open files (errno = 24).The cause is in
DBObjectDirectoryAdapter:selectAllandselectByIDsread all their object files in parallel, one asyncreadAsStringper file. Concurrent selects of a large table therefore opened thousands of files at once. This is not a leak: every file is closed afterwards. It is an unbounded burst.Investigation
ulimit -n 256: only theDBObjectDirectoryAdaptergroup fails. SQL memory, SQLite, object memory and fake GCS all pass.lsofsampling during that group:selectAlls of 1000 objects: 24000 open at once.Fix
DBObjectDirectoryAdapter: object reads go through aFileLimitedsemaphore (default 50), the same mechanismAPIServerResponseCachealready uses. The peak above drops from 3875 to 38.FileLimited.readAsString: likeFile.readAsString, under the semaphore. This adds one method to an exported class, matching its existingreadAsBytes/writeAsBytes/stat/delete/exists.Review fixes (third commit)
A
/code-reviewpass found 7 issues, and all are fixed:_readObjectcheckedexistsSyncbefore waiting for the semaphore, so an object deleted while its read waited threwPathNotFoundExceptionand failed the whole select.null._doDeleteImplnow reads synchronously instead of waiting behind the reads of concurrent selects. A concurrent delete of the same object now deletes nothing instead of throwing.FileLimitedsemaphore swap. Every method released whatever semaphore_semaphorepointed to at that moment, so an operation still running whensetSemaphoreLimitreplaced it released the new semaphore, pushing it above its limit._runhelper that releases the semaphore it acquired.DBObjectDirectoryAdapter.fileLimitedexposes the shared semaphore so it can be tuned withsetSemaphoreLimit, andFileLimitedis now exported bybones_api_db_directory.dart.fileLimitedlimit, so each concurrent select queues at most one batch of reads instead of one read per object.dart --packagesinstead ofdart run, so no package resolution or build hooks run under the low limit;ulimit -n 256can't be set.Tests
bones_api_entity_db_directory_test):selectAlls in a child process limited to 256 open files (ulimit -n 256), through the helper scripttest/bones_api_entity_db_directory_open_files.dart.errno = 24without the fix and passes with it./dev/fdlists at most 128 entries, against 24000 really open.masterworktree: on master it fails withToo many open files; on this branch it completesOK: 10 x 1000 objects.selectAlls run while 150 objects are each deleted twice at the same time;fileLimited: the default limit, and reads in batches of a tuned limit (selectAllandselectByIDs).FileLimited.readAsStringunit test: encoding, concurrency above the limit, release after an error.FileLimited.setSemaphoreLimitwhile operations are running: all of them complete, and the new semaphore keeps working.bones_api_concurrency_memory_test.dartnow passes withulimit -n 256.Run locally:
dart format --set-exit-if-changed: cleandart analyze --fatal-infos --fatal-warnings: cleandart test --platform vm -x docker -x concurrency -j 4): all passedbones_api_version_test: passeddart pub publish --dry-run: 0 warningsVersion
1.24.1, since v1.24.0 is tagged:
pubspec.yaml,BonesAPI.VERSIONand the CHANGELOG.#186 (static files
.aliases) is at 1.25.0. Whichever PR merges second will conflict on the version lines and the top of the CHANGELOG. If this one merges first, #186 stays 1.25.0, with the 1.24.1 section below it.🤖 Generated with Claude Code