Repository navigation
[Storage] Rewrite Firebase.Storage in pure C# - #1555
Open
AustinBenoit wants to merge 17 commits into
Open
AustinBenoit wants to merge 17 commits into
AustinBenoit wants to merge 17 commits into
Conversation
- Replace C++ SWIG wrapper (storage.i / FirebaseCppStorage) with a pure C# implementation in storage/src/ targeting Unity 2022.3+ (.NET Standard 2.1). - Extend shared Firebase.Internal (app/src/internal/FirebaseInterops.cs and HttpHelpers.cs) with AppDisposed reflection helpers, Storage REST headers (X-Firebase-Storage-Version, Authorization, x-firebase-appcheck, x-firebase-gmpid), and non-blocking exponential backoff with jitter. - Implement adaptive GCS resumable uploads (256 KB -> 32 MB chunks with ArrayPool<byte>.Shared, session URI exposure/resumption, and cancel support) and resumable Range + ETag validated downloads. - Enforce SemaphoreSlim concurrency gates (2 uploads, 3 downloads, 5 commands) and add 5 new pure-C# tests to UIHandlerAutomated.cs (45 total tests).
- GetDownloadUrlAsync: route the returned Uri through FirebaseStorage.ConstructFormattedUri so "%2F" in the object path is preserved by Uri.ToString()/PathAndQuery (matches the pre-rewrite SDK). - ResumableUploadTask: fix SynchronizeBufferOrStreamAsync so an out-of-buffer server offset re-reads a fresh chunk from the synchronized stream position (seekable and non-seekable), accounts for the lookahead byte, and fails clearly if a non-seekable stream ends before the server offset. Handle "final" query responses with an empty body by fetching metadata (ResolveFinalizedMetadataAsync). - ResumableDownloadTask: ETag mismatch on a streaming reconnect now fails fast instead of retrying until MaxDownloadRetryTime; handle a server that ignores Range and returns 200 by draining already-consumed bytes (erroring on short bodies); dispose the response if the body stream cannot be opened; delete a newly created destination file when GetFileAsync fails or is canceled. - ResolveLocalFilePath (upload + download): do not unescape Uri.LocalPath a second time. - TransferStateUpdater: log progress-handler exceptions directly instead of leaving them in ExceptionAggregator thread-local storage. - ModuleLogger: drop the unused strong-reference children list that leaked one logger per StorageReference; StorageReference.Logger is now lazy. - FirebaseStorage.GetInstance(app, url): tolerate a null app again. - unity_packer/debug_single_export_json/storage.json: match exports.json. - Tests: TestUploadSessionUriAndResume now actually resumes from the captured session URI; add TestUploadPartialSessionResume which interrupts a non-seekable stream upload after the first chunk and resumes it. Mono desktop suite: 46/46 passing.
- Release download concurrency slot after response headers in GetStreamAsync() and add finalizer to ResumableDownloadStream (B1) - Dispatch IProgress<T> callbacks on main thread via FirebaseHandler.RunOnMainThreadAsync with coalescing and FlushAsync (B2) - Add 60s per-attempt StallWatchdog and WatchdogByteArrayContent for transfer stall detection (B3) - Reject operations on RootReference and empty buckets with StorageException (B4) - Document 14.0.0 Storage rewrite and migration notes in docs/readme.md and XML comments (B5) - Preserve null values in MetadataChange/StorageMetadata to support clearing fields and deleting custom metadata keys (S1) - Increase HttpClientHandler.MaxConnectionsPerServer to 16 (S2) - Preserve inner network exceptions on retry budget exhaustion and rethrow non-network exceptions (S3) - Respect initial Stream.Position for seekable streams in PutStreamAsync (S5) - Route non-user OperationCanceledExceptions through backoff instead of pause branch (S6) - Add ModuleLogger diagnostics and consolidate StorageHttpClient.SendCommandAsync (S7) - Subscribe directly to FirebaseApp.AppDisposed, restore GetInstance(FirebaseApp, string = null) overload, and remove dead reflection fallback (S8, S9, S12) - Cap ETag mismatch restarts and write GetFileAsync via atomic .fbdownload temp file (S10, S11) - Add integration tests for root reference rejection, metadata deletion, undisposed GetStreamAsync, non-zero stream position, and GetFileAsync failure preservation (S13)
…ase notes to Upcoming Changes
…unrelated module diffs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request rewrites the internal implementation of the Firebase Storage Unity SDK in pure C#, completely removing the dependency on the C++ SDK and native platform Storage libraries. It introduces new C# classes to handle HTTP REST communication, resumable uploads/downloads, concurrency scheduling, and stall watchdogs. Additionally, it updates metadata, progress tracking, and exception handling to work natively in C#, and adds comprehensive integration tests. Feedback on the changes highlights a potential race condition in 'FirebaseStorage.cs' between 'Dispose()' and 'GetInstance()' when accessing the cached instances dictionary, and suggests synchronizing these operations with a lock.
AustinBenoit
added
the
tests-requested: full
…isposed outside cache lock, unique GetFile temp names, 404 bucket/project mapping, exponential backoff for transport errors, multi-chunk test
…off for retryable HTTP statuses
…transfers, with tests
…se/cancel/retry paths work on Windows Mono
- Retry budgets (MaxUploadRetryTime / MaxDownloadRetryTime) now measure time spent failing rather than wall-clock time: the stopwatch starts on the first failure and resets when bytes flow or after a pause, so long healthy transfers and long pauses no longer exhaust the budget on the next blip. - Paused transfers hand their scheduler slot back (StorageTaskScheduler.Slot) so two paused uploads / three paused downloads cannot starve other transfers. - Pause() now aborts an in-flight read on GetStreamAsync streams too. - Cancellation always wins over the retry budget in SendWithExponentialBackoffAsync (Mono reports a cancelled request as WebException). - StorageTask caches its CancellationToken so it stays queryable after Dispose. - StorageController records Pause()/Cancel() made after its previous transfer finished, matching the documented pause-before-start behavior. - ResumableDownloadStream registers a weak cancel callback (undisposed streams stay collectable) and never throws from its finalizer. - Per-instance FirebaseStorage logger (tagged with the instance URL) restored. - Removed legacy C++ error-code mapping and other dead code; dropped unrelated app folder GUIDs from guids.json and aligned internal.meta with guids.json. - Fixed TestGetFileAsyncPreservesExistingFileOnFailure temp-file assertion and added TestPausedTransfersDoNotBlockOthers.
…sfer has finished, as documented and tested
…re Unity-generated internal.meta GUID
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.
Description
Rewrites
Firebase.Storagein pure C# on top of the Firebase Storage REST API, removing the dependency on the C++ SWIG layer and native platform Storage libraries (FirebaseCppStorage, Androidfirebase-storage-unity, and iOSFirebaseStorage).Firebase.Storageas source underAssets/Firebase/FirebaseStoragewithFirebase.Storage.asmdef(matchingFirebase.FunctionsandFirebase.FirebaseAI), and removes the hard package dependency onFirebase.Auth(Auth and App Check tokens are attached viaFirebaseInteropswhen present).256 KBto32 MB) and populateUploadState.UploadSessionUriso interrupted uploads can be resumed viapreviousSessionUrion all platforms.Rangeresumption withETagverification, andGetStreamAsync()now returns a live, forward-only network stream instead of buffering the entire object into aMemoryStream.MaxUploadRetryTime,MaxDownloadRetryTime, andMaxOperationRetryTime.StorageController(mirrors the C++ SDK'sfirebase::storage::Controllerand thepause()/resume()/cancel()on Android/iOS/Web transfer tasks). Pass one as the new trailing optionalcontrollerparameter ofPutBytesAsync/PutFileAsync/PutStreamAsync/GetBytesAsync/GetFileAsync/GetStreamAsync(Action<Stream>, ...). Paused uploads resume from the server-committed offset; paused downloads resume with aRangerequest.CancellationTokencontinues to work as before.Firebase.Storagepublic API surface; the only additions areStorageControllerand the trailing optionalcontrollerparameter above (source-compatible with all existing calls).Changelog
UploadState.UploadSessionUrion all platforms so interrupted uploads can be resumed via thepreviousSessionUriparameter ofPutBytesAsync,PutFileAsync, andPutStreamAsync.StorageController, which can be passed to the upload and download methods onStorageReferenceto pause, resume or cancel a transfer in progress.StorageReference.GetStreamAsync()now returns a live, forward-only network stream instead of a bufferedMemoryStream(CanSeekisfalse).Testing
Type of Change
Place an
xthe applicable box: