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

webdav: nextcloud: new chunking code causing a panic #7168

Closed
ncw opened this issue Jul 24, 2023 · 7 comments · Fixed by #7195
Closed

webdav: nextcloud: new chunking code causing a panic #7168

ncw opened this issue Jul 24, 2023 · 7 comments · Fixed by #7195

Comments

@ncw
Copy link
Member

ncw commented Jul 24, 2023

It was reported on the forum that rclone v1.63.1 is panicing in the Go http standard library

https://forum.rclone.org/t/panic-after-updating-to-v1-63-1/40226

With a bit of tracing the Go stdlib code I managed to work out that it is caused by the nil, nil return of this code

getBody := func() (io.ReadCloser, error) {
// RepeatableReader{} plays well with accounting so rewinding doesn't make the progress buggy
if _, err := in.Seek(0, io.SeekStart); err == nil {
return nil, err
}

GetBody must not return a nil body with a nil error.

@devnoname120 do you have any ideas about this? I think this is probably a type which should say err != nil not err == nil but that means we've been taking the wrong branch all along so a bit of testing may be needed.

How to use GitHub

  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.
@devnoname120
Copy link
Contributor

devnoname120 commented Jul 25, 2023

@ncw Sorry for the issue and thanks for the ping, I'll try to look into it later this week.

From rough memory I remember that I specifically added and tested this retry mechanism because it was preventing me from uploading large files (50 GB) with a large number of chunks (~ 1000).

There was almost always at least a chunk that failed to upload and would trigger the restart of the entire transfer. So it definitely worked at some point because I personally needed this to work. I may have pushed the wrong commit or something at that time however, so I will investigate...

@ncw
Copy link
Member Author

ncw commented Jul 26, 2023

I'm pretty sure changing err == nil to err != nil will fix it but great if you can confirm.

devnoname120 added a commit to devnoname120/rclone that referenced this issue Jul 31, 2023
Fix rclone#7168

Co-authored-by: ncw <nick@craig-wood.com>
Co-authored-by: Paul <devnoname120@gmail.com>
@devnoname120
Copy link
Contributor

@ncw Your suggestion looks good to me. It indeed looks like a mistake — maybe I botched up at some point when refactoring my pull request?

I opened a new pull request, it also fixes an additional segmentation violation: #7195

@ncw ncw closed this as completed in #7195 Aug 1, 2023
ncw added a commit that referenced this issue Aug 1, 2023
Fix #7168

Co-authored-by: ncw <nick@craig-wood.com>
Co-authored-by: Paul <devnoname120@gmail.com>
@ncw
Copy link
Member Author

ncw commented Aug 1, 2023

I've merged this to master now which means it will be in the latest beta in 15-30 minutes and released in v1.64 (or v1.63.2 if we make one)

Thank you @devnoname120

leekiernan pushed a commit to leekiernan/rclone that referenced this issue Aug 24, 2023
Fix rclone#7168

Co-authored-by: ncw <nick@craig-wood.com>
Co-authored-by: Paul <devnoname120@gmail.com>
@devnoname120
Copy link
Contributor

@ncw I'm not entirely sure how your release schedule works but do you plan to publish a new release in the near future? Imho those fixes are significant.

@ncw
Copy link
Member Author

ncw commented Sep 2, 2023

@devnoname120 check out the milestone :-) In about a week is the plan!

@ncw
Copy link
Member Author

ncw commented Sep 12, 2023

This is released into v1.64 now :-)

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

Successfully merging a pull request may close this issue.

2 participants