Fix resumable upload error "There was a problem uploading your video" #1001

phaizullin · 2018-06-01T11:29:41Z

Changed start_offset and end_offset from error code as written in documentation

facebook-github-bot · 2018-06-01T11:30:42Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2018-06-01T11:41:21Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

yguedidi · 2018-06-17T09:59:01Z

@phaizullin Thanks for your contribution!
Why would you like include this behavior in the SDK? Especially that you can do the same in your code by allowing to throw and catching the exception

phaizullin · 2018-06-18T07:33:23Z

@yguedidi Firstly, because it is mostly facebook internal error, it want to process another chunk (mostly random chunk) after processing current chunk, but uploadVideo method retry only current chunk and it returns same error 5 times via maxTriesTransfer method.
I think that main function of sdk is minimize interactions between user code and api. If api wants to upload another chunk it must do it.
Secondly, if I catch the error, I will need to re-implement Facebook class methods

nasy · 2018-06-18T09:00:21Z

I am also experiencing the same problems, it would be nice for this fix to get merged.

abhishekbh · 2018-07-02T21:39:13Z

This is an important patch. What's the hold up in the merge or further comments?

SammyK · 2018-07-03T02:17:53Z

This seems to be a common issue so I'd be +1 for merging. For the record, the error message for 1363037 is:

There was a problem uploading your video. Please try uploading it again. error_data:{start_offset:0,end_offset:392695}

The recommendation is:

Returned when the server receives an invalid start offset from the client. Try to parse the start_offset and end_offset from error message and do a retry again.

@phaizullin & @yguedidi Assuming we merge this in - we should probably figure out how this impacts the max retires. This PR technically adds a non-deterministic outcome as there is an edge case where Graph returns the 1363037 sub code every time. Wudya think? :)

SammyK

@phaizullin Thanks for the PR! Can we also add a test to show this functionality works? Once we get these things tweaked, we can merge this in. :)

SammyK · 2018-07-03T02:19:11Z

src/Facebook/FileUpload/FacebookResumableUploader.php

+                    $chunk->getFile(),
+                    $chunk->getUploadSessionId(),
+                    $chunk->getVideoId(),
+                    $e->getResponseData()['error']['error_data']['start_offset'],


We should probably do some isset() checks on this and the next line just in case Graph changes the error data structure it responds with.

SammyK · 2018-07-03T02:19:32Z

src/Facebook/FileUpload/FacebookResumableUploader.php

@@ -121,6 +121,16 @@ public function transfer($endpoint, FacebookTransferChunk $chunk, $allowToThrow
                throw $e;
            }

+            if ($e->getSubErrorCode() === 1363037) {


Let's make 1363037 a class constant. :)

yguedidi · 2018-07-03T06:22:13Z

@phaizullin @SammyK I'm not that against this PR, I'm more against the implementation.

Let met explian: This solution looks more like a quick workaround to the issue.

How I conceptualy see it is more: you should catch a specific exception, that will give you access to a getRetryableChunk() or somethig like that, and you do what you want with it.

Enforcing an error handling internally looks bad to me. An SDK responsability is to provide the more detailed error, while handle it is user's code responsability: an SDK shouldn't has error no? :)

phaizullin · 2018-07-03T23:05:39Z

@yguedidi @SammyK thanks for review. Please see the changes one more time)

abhishekbh · 2018-07-04T01:34:32Z

@yguedidi what you're saying makes sense, however the current pattern the SDK sets up with the "super" Facebook class is that the client has a minimum amount of information about the upload process.

If the FB SDK expects the calling client to handle chunks and errors (not a bad idea), it should expose more methods to allow an efficient way of doing this. The only public method in the Facebook class at the moment is "uploadVideo".

In the meanwhile, is there another recommendation on working around this issue?

SammyK · 2018-07-06T16:43:10Z

@abhishekbh You could work directly with the Facebook\FileUpload\FacebookResumableUploader class to better handle these cases, but it doesn't have retry logic built in since that's handled by the main Facebook service class. I'd love hear if @yguedidi has a better suggestion for handling this particular case though. :)

yguedidi · 2018-07-10T19:26:27Z

How I see things:

FacebookResumableUploader is the pure way, it does one try, if it fails, it throws
uploadVideo() is a helper function, a convenient way to do a full upload in one run, with cool feature like auto-retry.

I see the proposed fix (return the retryable chunk), as one of those cool feature, so I think it should live in the uploadVideo() function and be counted in the number of tries. :)
So basicly, what annoys me here is that it's the FacebookResumableUploader who handle that special case.

phaizullin · 2018-07-24T08:29:34Z

@yguedidi It is't entirely clear from your words what changes in the PR are needed.

yguedidi

let's accept this solution and move forward! it still can be refactored for v6.
please rebase, fix review and update the changelog

yguedidi · 2018-11-14T07:24:05Z

src/Facebook/Exceptions/FacebookResponseException.php

+                    $previousException = new FacebookResumableUploadException($message, $code);
+
+                    $startOffset = isset($data['error']['error_data']['start_offset']) ?
+                        $data['error']['error_data']['start_offset'] : null;


please keep this in the same line. BTW, you should cast to int the offset

yguedidi · 2018-11-14T07:24:10Z

src/Facebook/Exceptions/FacebookResponseException.php

+                    $previousException->setStartOffset($startOffset);
+
+                    $endOffset = isset($data['error']['error_data']['end_offset']) ?
+                        $data['error']['error_data']['end_offset'] : null;


please keep this in the same line. BTW, you should cast to int the offset

yguedidi · 2018-11-14T07:24:43Z

src/Facebook/Exceptions/FacebookResumableUploadException.php

+    protected $endOffset;
+
+    /**
+     * @return int


yguedidi · 2018-11-14T07:24:48Z

src/Facebook/Exceptions/FacebookResumableUploadException.php

+    }
+
+    /**
+     * @param int $startOffset


yguedidi · 2018-11-14T07:24:53Z

src/Facebook/Exceptions/FacebookResumableUploadException.php

+    }
+
+    /**
+     * @return int


yguedidi · 2018-11-14T07:24:58Z

src/Facebook/Exceptions/FacebookResumableUploadException.php

+    }
+
+    /**
+     * @param int $endOffset


yguedidi · 2018-11-14T07:26:07Z

src/Facebook/FileUpload/FacebookResumableUploader.php

@@ -121,6 +121,16 @@ public function transfer($endpoint, FacebookTransferChunk $chunk, $allowToThrow
                throw $e;
            }

+            if (!is_null($preException->getStartOffset()) && !is_null($preException->getEndOffset())) {


I'd prefer !== null instead of is_null

yguedidi · 2018-11-14T07:26:36Z

tests/FileUpload/FacebookResumableUploaderTest.php

+
+        $chunk = new FacebookTransferChunk($this->file, '1', '2', '3', '4');
+        $newChunk = $uploader->transfer('/me/videos', $chunk);
+        $this->assertEquals('40', $newChunk->getStartOffset());


please assert also on the end ofset

yguedidi · 2018-11-15T07:43:20Z

@phaizullin please rebase so that only your commit appear in the diff

yguedidi · 2018-11-19T21:36:34Z

@phaizullin any news?

… Please try again.

…or resumable upload.

changes done

yguedidi · 2018-11-21T07:16:10Z

Thank you @phaizullin

yguedidi · 2018-11-21T07:16:54Z

And congrats for you first contribution to the SDK @phaizullin, I can't wait for your other pull requests!

facebook-github-bot added the CLA Signed label Jun 1, 2018

This was referenced Jun 16, 2018

Problems trying to upload heavy videos (2 min, 300MB aprox) by chunks #1012

Closed

Resumable upload to fb page returns error "There was a problem uploading your video. Please try again." code : 6001 #1000

Closed

SammyK added this to the 5.6.3 milestone Jul 3, 2018

SammyK previously requested changes Jul 3, 2018

View reviewed changes

SammyK modified the milestones: 5.6.3, 5.7.0 Jul 3, 2018

yguedidi suggested changes Nov 14, 2018

View reviewed changes

phaizullin force-pushed the 5.x branch from 8a30550 to 8512193 Compare November 20, 2018 17:05

phaizullin closed this Nov 20, 2018

phaizullin force-pushed the 5.x branch from 8512193 to 6d1eb5a Compare November 20, 2018 19:00

phaizullin added 2 commits November 20, 2018 22:12

Fix resumable upload error "There was a problem uploading your video.…

dd8f1a7

… Please try again.

Added offset fields to FacebookResumableUploadException. Added test f…

179eec3

…or resumable upload.

phaizullin reopened this Nov 20, 2018

Update CHANGELOG for facebookarchive#1001 and fixed review

b4ee993

yguedidi approved these changes Nov 21, 2018

View reviewed changes

yguedidi merged commit 4f1c91f into facebookarchive:5.x Nov 21, 2018

Fix resumable upload error "There was a problem uploading your video" #1001

Fix resumable upload error "There was a problem uploading your video" #1001

Uh oh!

Conversation

phaizullin commented Jun 1, 2018

Uh oh!

facebook-github-bot commented Jun 1, 2018

Uh oh!

facebook-github-bot commented Jun 1, 2018

Uh oh!

yguedidi commented Jun 17, 2018

Uh oh!

phaizullin commented Jun 18, 2018

Uh oh!

nasy commented Jun 18, 2018

Uh oh!

abhishekbh commented Jul 2, 2018

Uh oh!

SammyK commented Jul 3, 2018

Uh oh!

SammyK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yguedidi commented Jul 3, 2018

Uh oh!

phaizullin commented Jul 3, 2018

Uh oh!

abhishekbh commented Jul 4, 2018

Uh oh!

SammyK commented Jul 6, 2018

Uh oh!

yguedidi commented Jul 10, 2018

Uh oh!

phaizullin commented Jul 24, 2018

Uh oh!

yguedidi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yguedidi commented Nov 15, 2018

Uh oh!

yguedidi commented Nov 19, 2018

Uh oh!

yguedidi commented Nov 21, 2018

Uh oh!

yguedidi commented Nov 21, 2018

Uh oh!

Uh oh!