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

Allow XRootD to return trailers indicating failure #1912

Merged
merged 2 commits into from Feb 23, 2023

Conversation

bbockelm
Copy link
Contributor

HTTP provides a response to include a "trailer" in addition to the better-known "header". If a user sets the following headers in the request:

X-Transfer-Status: true
TE: trailers
Transfer-Encoding: chunked

Then the response will used chunked encoding and indicate, on the last returned chunk, whether an error has occurred.

Clients aware of these headers can now receive an error message from XRootD if there's an IO error in the middle of the response. This is expected to be useful in XCache use cases where failure mid-response is somewhat more common.

HTTP provides a response to include a "trailer" in addition to the
better-known "header".  If a user sets the following headers in
the request:

```
X-Transfer-Status: true
TE: trailers
Transfer-Encoding: chunked
```

Then the response will used chunked encoding and indicate, on the
last returned chunk, whether an error has occurred.

Clients aware of these headers can now receive an error message from
XRootD if there's an IO error in the middle of the response.  This is
expected to be useful in XCache use cases where failure mid-response
is somewhat more common.
@bbockelm
Copy link
Contributor Author

@djw8605 -- the intent is the stashcp / stash_plugin client should be able to use this information to build better information for XCache requests.

@ccaffy
Copy link
Contributor

ccaffy commented Feb 17, 2023

Hi @bbockelm ,

Thanks for this :)

I compiled everything and it works in the case the transfer succeeds. Out of curiosity, how do you manage to test that functionality ?

I tried to fail the transfer but I get no information from the body/trailers that it fails, so probably I am missing something.

@bbockelm
Copy link
Contributor Author

@ccaffy - I test with this small patch to inject a fault:

--- a/src/XrdOss/XrdOssApi.cc
+++ b/src/XrdOss/XrdOssApi.cc
@@ -884,8 +884,13 @@ ssize_t XrdOssFile::Read(void *buff, off_t offset, size_t blen)
            else   retval = cxobj->Read((char *)buff, blen, offset);
         else 
 #endif
+     if (offset < 64*1024) {
              do { retval = pread(fd, buff, blen, offset); }
                 while(retval < 0 && errno == EINTR);
+     } else {
+        errno = EIO;
+        retval = -1;
+     }
 
      return (retval >= 0 ? retval : (ssize_t)-errno);
 }

Any reads after 64KB will fail (though, since reads are issued as 1MB chunks, the test file needs to be >1MB).

Note that you'll need to add --raw -v to the curl command to see the trailer. Sadly trailers aren't printed by -v by default.

Additionally does some modest tidying of comments.
@ccaffy
Copy link
Contributor

ccaffy commented Feb 22, 2023

Hi @bbockelm ,

Thanks for the tips :) I managed to have the trailer working indeed. If I add your snippet of code, I indeed get the trailer:

X-Transfer-Status: 500: Unable to read /tmp/bigfile_5M_copy; input/output error

Is it normal that I get a 200 OK response though ?

> GET /tmp/bigfile_5M_copy HTTP/1.1
> User-Agent: curl/7.29.0
> Host: xrootd-ccaffy-dev01.cern.ch:1096
> Accept: */*
> X-Transfer-Status: true
> TE: trailers
> Transfer-Encoding: chunked
> 
< HTTP/1.1 200 OK
< Connection: Keep-Alive
< Server: XrootD/v5.2.0-99-osghotfix...687
< Content-Length: 5000000
< Transfer-Encoding: chunked
< 
{ [data not shown]
 20 4882k   20 1024k    0     0  6266k      0 --:--:-- --:--:-- --:--:-- 6282k
[FILE CONTENT]

X-Transfer-Status: 500: Unable to read /tmp/bigfile_5M_copy; input/output error


* Connection #0 to host xrootd-ccaffy-dev01.cern.ch left intact

@bbockelm
Copy link
Contributor Author

Is it normal that I get a 200 OK response though ?

Yeah -- this is a typical issue in HTTP where the HTTP status has to be sent before the body. So, if there's any I/O error, it's too late to change the response status code.

Currently, when an error happens mid-transfer, the server simply closes the TCP connection and the client reports something like "unexpected EOF". With this patch, if you ask for a status trailer, the client can get a useful error message (you might notice other PRs to improve error messages -- it's all related!).

This has to be opt-in, however, to prevent unexpecting clients from interpreting the early exit as a success as the response no longer generates TCP-level problems.

@ccaffy
Copy link
Contributor

ccaffy commented Feb 22, 2023

Alright, all things are clear now :)
Thanks for your detailed explanations that allowed me to learn ;)
Approving this PR...

@abh3 abh3 merged commit 56b2bde into xrootd:master Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants