Skip to content
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

Fix streaming uploads #1406

Merged
merged 11 commits into from
Mar 19, 2024
Merged

Fix streaming uploads #1406

merged 11 commits into from
Mar 19, 2024

Conversation

jbrichau
Copy link
Member

This PR fixes two problems with streaming file uploads:

  • The streaming file uploads in GemStone did not work and only uploaded the last bytes of a file
  • Changing the 'streamUploads' option on a ZnServer required to also manually call #configureServerForBinaryReading on the adapter, otherwise no streaming was done anyway.

@jbrichau jbrichau requested a review from theseion March 17, 2024 16:45
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.71%. Comparing base (33d0e82) to head (df0dc34).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1406      +/-   ##
==========================================
+ Coverage   48.49%   48.71%   +0.22%     
==========================================
  Files        9164     9166       +2     
  Lines       82164    82191      +27     
==========================================
+ Hits        39847    40041     +194     
+ Misses      42317    42150     -167     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@theseion
Copy link
Member

I don't understand the fix for the missing bytes. As far as I can tell, the change would even break things. The collection used as buffer for the ring buffer will not start at 1 necessarily. Using the raw buffer as argument to #nextPutAll: will send #do: which would iterate from 1 to n over the raw buffer. What should be done, IIRC, is that the #do: message of ZnRingBuffer is sent (hence the ring buffer instance was passed to #nextPutAll:). ZnRingBuffer>>#do: should iterate over the raw buffer from start to finish, regardless of where the start index points to in the raw buffer.

@jbrichau
Copy link
Member Author

jbrichau commented Mar 17, 2024

I don't understand the fix for the missing bytes.

The implementation of GsFile>>nextPutAll: is such that passing a ZnRingBuffer rather than a Collection does not work. It sends _basicSize to the instance to determine how many elements it should copy. For a ZnRingBuffer the result is 0, which means nothing is written at all to the file. Only the last few bytes which are written using the iteration that sendsat: in the other branch of the conditional statement in ZnStreamingMultiPartFormDataEntity>>#parseMultiPartFieldWithoutLengthWithBoundary:writeOn: where the nextPutAll: is sent.
The implementation of delegating the writing for the entire buffer directly to the internal collection fixes that. But...

As far as I can tell, the change would even break things. The collection used as buffer for the ring buffer will not start at 1 necessarily. Using the raw buffer as argument to #nextPutAll: will send #do: which would iterate from 1 to n over the raw buffer. What should be done, IIRC, is that the #do: message of ZnRingBuffer is sent (hence the ring buffer instance was passed to #nextPutAll:). ZnRingBuffer>>#do: should iterate over the raw buffer from start to finish, regardless of where the start index points to in the raw buffer.

You're right. I did a number of tests that worked fine and assumed too quickly it would be ok.
I guess the place where the entire buffer is sent in the current implementation indeed always works... . But I will check and what you mention makes sense. I'll get back to this...

@jbrichau
Copy link
Member Author

@theseion corrected the implementation. Thanks!

@jbrichau
Copy link
Member Author

Thanks for the review @theseion !

@jbrichau jbrichau merged commit 63fbd49 into master Mar 19, 2024
18 of 24 checks passed
@jbrichau jbrichau deleted the fix-streaming-uploads-setting branch March 19, 2024 20:17
@theseion
Copy link
Member

Thank you! :)

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