SeasideSt / Seaside

The framework for developing sophisticated web applications in Smalltalk.
MIT License
519 stars 71 forks source link

Fix streaming uploads #1406

Closed jbrichau closed 8 months ago

jbrichau commented 8 months ago

This PR fixes two problems with streaming file uploads:

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

theseion commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@theseion corrected the implementation. Thanks!

jbrichau commented 8 months ago

Thanks for the review @theseion !

theseion commented 8 months ago

Thank you! :)