Skip to content

Comments

[feature] Adjust temporary upload settings#191

Merged
Ghass-M merged 5 commits intomasterfrom
feature/adjust-temporary-initial-upload-settings
Aug 25, 2025
Merged

[feature] Adjust temporary upload settings#191
Ghass-M merged 5 commits intomasterfrom
feature/adjust-temporary-initial-upload-settings

Conversation

@Ghass-M
Copy link
Contributor

@Ghass-M Ghass-M commented Aug 25, 2025

Summary

Reduce initial upload parallelism and slice size to improve stability on first-run uploads and lower resource usage on constrained networks/devices.

Changes

  • File: src/ByteSync.Client/Services/Communications/Transfers/Uploading/FileUploadProcessor.cs
    • Decrease upload workers from 6 to 2.
  • File: src/ByteSync.Client/Services/Encryptions/SlicerEncrypter.cs
    • Set MaxSliceLength to 1 MB (was effectively 4 MB).
    • Remove unused System.Threading.Tasks using.
    • Clean up duplicate/overwritten MaxSliceLength assignment.

Rationale

  • Reduce simultaneous slice uploads to avoid saturating bandwidth and hitting server throttles during initial sessions.
  • Smaller slice size:
    • Lowers peak memory usage and retry costs.
    • Improves resume granularity and fairness across members.
    • Better behavior on variable/slow connections.

Impact

  • Expected: smoother first-time upload experience with lower CPU/memory/network spikes.
  • Trade-off: potential reduction in peak throughput; increased number of requests for large files.
  • No breaking API changes.

@Ghass-M Ghass-M requested a review from paul-fresquet August 25, 2025 09:37
@paul-fresquet paul-fresquet requested a review from Copilot August 25, 2025 09:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reduces upload parallelism and slice size to improve stability and resource usage during file uploads. The changes focus on making the initial upload experience more stable on constrained networks and devices.

  • Reduced upload worker count from 6 to 2 to prevent bandwidth saturation
  • Decreased maximum slice size from 4MB to 1MB for better memory usage and resume granularity
  • Code cleanup including fixing variable naming, removing unused imports, and improving error handling

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
FileUploadProcessor.cs Reduced concurrent upload workers from 6 to 2 and improved exception type specificity
SlicerEncrypter.cs Set slice size to 1MB, fixed variable naming, removed unused import, and improved async handling
FileUploadProcessorTests.cs Updated test to verify 2 workers instead of 6 and added null-forgiving operators

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 106 to 109
using (var cts = new CancellationTokenSource())
{
await cryptoStream.WriteAsync(bytes.AsMemory(0, readBytes), cts.Token);;
}
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

The CancellationTokenSource is created but never used for actual cancellation. Either remove the using block and pass CancellationToken.None, or connect it to a proper cancellation mechanism. Also, there's a double semicolon at the end of line 108.

Suggested change
using (var cts = new CancellationTokenSource())
{
await cryptoStream.WriteAsync(bytes.AsMemory(0, readBytes), cts.Token);;
}
await cryptoStream.WriteAsync(bytes.AsMemory(0, readBytes), CancellationToken.None);

Copilot uses AI. Check for mistakes.
@sonarqubecloud
Copy link

Copy link
Contributor

@paul-fresquet paul-fresquet left a comment

Choose a reason for hiding this comment

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

👍

@Ghass-M Ghass-M merged commit 3c25e92 into master Aug 25, 2025
24 checks passed
@Ghass-M Ghass-M deleted the feature/adjust-temporary-initial-upload-settings branch August 25, 2025 10:20
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.

2 participants