Retries now send the file correctly. #153
Conversation
|
@adam-vessey did you miss a commit? |
HttpConnection.php
Outdated
| @@ -689,7 +692,7 @@ function putRequest($url, $type = 'none', $file = NULL) { | |||
| // Ugly substitute for a try catch finally block. | |||
| $exception = NULL; | |||
| try { | |||
| $results = $this->doCurlRequest(); | |||
| $results = $this->doCurlRequest(NULL, $fh); | |||
There was a problem hiding this comment.
In the case that the type of the put request is 'none', $fh isn't defined here.
There was a problem hiding this comment.
@jonathangreen @adam-vessey the absence of $fh seems to be handled by the argument 's default
This seems to consistent with the pre-pull way of doing it for $file, but I feel that maybe being explicit on a NULL $fh on type none would not harm at all.
There was a problem hiding this comment.
@adam-vessey there is a missing pull for sure regarding tests that is making TRAVIS fail. @willtp87 is that the one you are referring to?
... Was misdirected when the retry stuff was originally rolled.
|
@adam-vessey this looks good. Will wait a few hours in case @jonathangreen has some comments. Will merge after that or today last hour in no comments. Thaks |
|
Looks good to me. |
|
@adam-vessey and @jonathangreen thanks. Merged and JIRA ticket marked as Fixed. |
JIRA Ticket: ISLANDORA-1901
What does this Pull Request do?
Prevent timeouts in the event
PUTrequests fail due to spurious HTTP 409 Conflict errors.What's new?
Rewind the file handle when retrying.
How should this be tested?
Regression testing is about it... Spurious issues are spurious.
I've not yet actually tried, but I believe the timeout behaviour described in the ticket can be forced by making bad requests... which is to say, making a request with old last modified dates, resulting in an actual 409... So this PR would fail faster, avoiding the timeouts at least.
Additional Notes:
Interested parties
Maintainers: @DiegoPino and @willtp87