Skip to content

Make the file download delegate sendable #834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Apr 28, 2025

Motivation:

Delegates can be passed from any thread and are executed on an arbitrary event loop. That means they need to be Sendable. Rather than making them all Sendable in one go, we'll do the larger ones separately.

Modifications:

  • Make FileDownloadDelegate sendable

Result:

Safe to pass FileDownloadDelegate across isolation domains

@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Apr 28, 2025
@glbrntt glbrntt marked this pull request as draft April 28, 2025 16:18
Motivation:

Delegates can be passed from any thread and are executed on an arbitrary
event loop. That means they need to be Sendable. Rather than making them
all Sendable in one go, we'll do the larger ones separately.

Modifications:

- Make FileDownloadDelegate sendable

Result:

Safe to pass FileDownloadDelegate across isolation domains
@glbrntt glbrntt force-pushed the strict-file-download-delegate branch from a7935fe to cf6f391 Compare April 29, 2025 14:51
@glbrntt
Copy link
Collaborator Author

glbrntt commented Apr 29, 2025

API breakage expected:

3 breaking changes detected in AsyncHTTPClient:
  💔 API breakage: constructor FileDownloadDelegate.init(path:pool:reportHead:reportProgress:) is now with @preconcurrency
  💔 API breakage: constructor FileDownloadDelegate.init(path:pool:reportHead:reportProgress:) is now with @preconcurrency
  💔 API breakage: constructor FileDownloadDelegate.init(path:reportHead:reportProgress:) is now with @preconcurrency

@glbrntt glbrntt marked this pull request as ready for review April 29, 2025 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants