Skip to content

[wip] streaming uploads #6161

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 4 commits into
base: main
Choose a base branch
from

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Feb 16, 2023

motivation: improve upload to stream the content of the files instead of loading them to memory

changes:

  • introduce new upload http request type
  • implement streaming upload handling with URLSession
  • implement an upload stream provider for registry publishing

TODO:

  • add tests
  • figure out runloop issue

@tomerd tomerd force-pushed the feature/registry-publish-7 branch 2 times, most recently from 07807cd to 238a77f Compare February 16, 2023 08:37
self.dataToWrite = .none
inputStream.close()
} else {
let bytesWritten = self.outputStream.write(buffer, maxLength: bytesRead)
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens with the bytes that didn't get written? (i.e. bytesWritten < bytesRead)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch. is there a way to move the reader index back? otherwise I can buffer them

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you can't unread. You'll need to buffer them and you'll need to stop reading until you're done with the previous writes.

}
completionHandler(task.inputStream)
// FIXME: serious hack here
RunLoop.current.run(until: Date(timeIntervalSinceNow: 10000.0))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weissi any ideas on why the stream is not going notified unless I run the .current runloop manually?

Copy link
Contributor

Choose a reason for hiding this comment

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

that almost certainly means that you're blocking the runloop somewhere.

@tomerd tomerd force-pushed the feature/registry-publish-7 branch 2 times, most recently from 17c41bc to 04c0c7b Compare February 18, 2023 02:42
@tomerd tomerd self-assigned this Feb 21, 2023
motivation: improve upload to stream the content of the files instead of loading them to memory

changes:
* introduce new upload requiest type
* implement stream upload hanlding with URLSession
* implement an upload stream provider for registry publishing

TODO:
* add tests
@tomerd tomerd force-pushed the feature/registry-publish-7 branch from 04c0c7b to e896adf Compare February 24, 2023 18:57
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