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

lazy open first source stream in assemblystream #11980

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

icewind1991
Copy link
Member

This prevents a remove first chunk timing out when the pre-hooks take a long time

Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Oct 22, 2018
@icewind1991 icewind1991 added this to the Nextcloud 15 milestone Oct 22, 2018
@@ -191,7 +192,7 @@ public function stream_flush() {
* @return bool
*/
public function stream_eof() {
return $this->pos >= $this->size || $this->currentStream === null;
return $this->pos >= $this->size || ($this->currentNode >= count($this->nodes) && $this->currentNode === null);
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this to me in words, because the code and variables make very little sense. The part before the || makes sense: if the position is bigger than the size it reached the end of the file. But the second part can never become true - is this really correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

the second part should never be true, but in cases where the chunking fails the number of bytes read can become lower than expected which would lead to an infinite loop with only the first clause.

This patch is to fix that specific issue, but I figured it's better to leave the extra check there in case a similar issue occurs in the future

Copy link
Member

Choose a reason for hiding this comment

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

shoudl being the keyword 😉

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke MorrisJobke merged commit 869df2e into master Oct 23, 2018
@MorrisJobke MorrisJobke deleted the assemblly-stream-lazy branch October 23, 2018 14:17
@MorrisJobke
Copy link
Member

@icewind1991 @rullzer I guess we need to backport this, right?

@icewind1991
Copy link
Member Author

stable14: #11994
stable13: #11997

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants