[pacman-dev,1/1] RFC: handle soft failure in downloads

Message ID 20210520210509.18460-1-list@eworm.de
State New
Headers show
Series [pacman-dev,1/1] RFC: handle soft failure in downloads | expand

Commit Message

Christian Hesse May 20, 2021, 9:05 p.m. UTC
From: Christian Hesse <mail@eworm.de>

A server can indicate a soft failure by returning 412 (Precondition
Failed). Only a warning (instead of error) message is logged and
download is retried with next server, without counting server error.

This http code can be used by servers that are not expected to be
complete, for example when serving a local cache.

Signed-off-by: Christian Hesse <mail@eworm.de>
---
 lib/libalpm/dload.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Christian Hesse May 21, 2021, 6:52 a.m. UTC | #1
Christian Hesse <list@eworm.de> on Thu, 2021/05/20 23:05:
> From: Christian Hesse <mail@eworm.de>
> 
> A server can indicate a soft failure by returning 412 (Precondition
> Failed). Only a warning (instead of error) message is logged and
> download is retried with next server, without counting server error.
> 
> This http code can be used by servers that are not expected to be
> complete, for example when serving a local cache.

I am open to changes, only important is that server error count does not
increase. So how about...

* http code: We need something >= 400 for the code to behave correctly.
  Nothing fits perfectly... Do you think anything else matches better? Or
  should we use a proprietary code 9xx?
* log message: Is wording and severity ok?
* Where to document the behavior?
Maarten de Vries May 21, 2021, 7:11 a.m. UTC | #2
On 21-05-2021 08:52, Christian Hesse wrote:
> Christian Hesse <list@eworm.de> on Thu, 2021/05/20 23:05:
>> From: Christian Hesse <mail@eworm.de>
>>
>> A server can indicate a soft failure by returning 412 (Precondition
>> Failed). Only a warning (instead of error) message is logged and
>> download is retried with next server, without counting server error.
>>
>> This http code can be used by servers that are not expected to be
>> complete, for example when serving a local cache.
> I am open to changes, only important is that server error count does not
> increase. So how about...
>
> * http code: We need something >= 400 for the code to behave correctly.
>    Nothing fits perfectly... Do you think anything else matches better? Or
>    should we use a proprietary code 9xx?
> * log message: Is wording and severity ok?
> * Where to document the behavior?


I would say that 412 is not the right status code to use for this, since 
it already has a very specific different purpose.

Another thing to consider is that normal server software won't know to 
generate a different error code, so it will just return a 404 anyway. If 
you need specific support from the server software, you are probably 
better off with a custom header: X-Pacman-Expected-Failure or something 
along those lines. Then you don't have to violate the HTTP standard to 
get the desired behavior.

Although from a user perspective it would probably be simpler to put 
this information in the pacman.conf than to configure their HTTP server.


Kind regards,

Maarten
Christian Hesse May 21, 2021, 7:25 a.m. UTC | #3
Maarten de Vries <maarten@de-vri.es> on Fri, 2021/05/21 09:11:
> On 21-05-2021 08:52, Christian Hesse wrote:
> > Christian Hesse <list@eworm.de> on Thu, 2021/05/20 23:05:  
> >> From: Christian Hesse <mail@eworm.de>
> >>
> >> A server can indicate a soft failure by returning 412 (Precondition
> >> Failed). Only a warning (instead of error) message is logged and
> >> download is retried with next server, without counting server error.
> >>
> >> This http code can be used by servers that are not expected to be
> >> complete, for example when serving a local cache.  
> > I am open to changes, only important is that server error count does not
> > increase. So how about...
> >
> > * http code: We need something >= 400 for the code to behave correctly.
> >    Nothing fits perfectly... Do you think anything else matches better? Or
> >    should we use a proprietary code 9xx?
> > * log message: Is wording and severity ok?
> > * Where to document the behavior?  
> 
> I would say that 412 is not the right status code to use for this, since 
> it already has a very specific different purpose.
> 
> Another thing to consider is that normal server software won't know to 
> generate a different error code, so it will just return a 404 anyway. If 
> you need specific support from the server software, you are probably 
> better off with a custom header: X-Pacman-Expected-Failure or something 
> along those lines. Then you don't have to violate the HTTP standard to 
> get the desired behavior.

Great idea... I will have a look at this.

> Although from a user perspective it would probably be simpler to put 
> this information in the pacman.conf than to configure their HTTP server.

What do you think this should look like?

To give some background: There's pacredir [0][1] which tries to re-distribute
database and package files for local network. Depending on whether or not the
file is available it returns a temporary redirect (307) with location or not
found (currently 404).
I want to update that to send another http code or - even better as you
suggested - add a http header to indicate the expected failure.

[0] https://archlinux.org/packages/community/x86_64/pacredir/
[1] https://github.com/eworm-de/pacredir

Patch

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index f6a4f012..7116cb03 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -515,13 +515,23 @@  static int curl_check_finished_download(CURLM *curlm, CURLMsg *msg,
 
 				if(!payload->errors_ok) {
 					handle->pm_errno = ALPM_ERR_RETRIEVE;
-					/* non-translated message is same as libcurl */
-					snprintf(payload->error_buffer, sizeof(payload->error_buffer),
+					if(payload->respcode == 412) {
+						/* A server can indicate a soft failure by returning 412
+						   (Precondition Failed). Only a debug message is logged and
+						   download is retried with next server, without counting
+						   server error. */
+						_alpm_log(handle, ALPM_LOG_WARNING,
+							_("file '%s' is currently not available from %s\n"),
+							payload->remote_name, hostname);
+					} else {
+						/* non-translated message is same as libcurl */
+						snprintf(payload->error_buffer, sizeof(payload->error_buffer),
 							"The requested URL returned error: %ld", payload->respcode);
-					_alpm_log(handle, ALPM_LOG_ERROR,
+						_alpm_log(handle, ALPM_LOG_ERROR,
 							_("failed retrieving file '%s' from %s : %s\n"),
 							payload->remote_name, hostname, payload->error_buffer);
-					server_soft_error(handle, payload->fileurl);
+						server_soft_error(handle, payload->fileurl);
+					}
 				}
 				if(curl_retry_next_server(curlm, curl, payload) == 0) {
 					(*active_downloads_num)++;