[XrdHttp] Fix HTTP protocol errors on failure #2443
Conversation
d6ca179 to
9e8fef6
Compare
9e8fef6 to
3c7ab30
Compare
dynamic-entropy
left a comment
There was a problem hiding this comment.
Planning to build on the XrdOssTests to improve the webdav error messages. Left a request to rename the test file.
|
@amadio - I realized this PR was still open when rebasing branches. Certainly protocol errors are a bug -- was there a reason it didn't make it into 5.8.2? Just an oversight? |
3c7ab30 to
80516ee
Compare
|
Yes, oversight, sorry. There were still unresolved conversations, so I wasn't sure this was ready or not. We can add this to the next patch release in ~2 weeks. I will tag it so I don't forget. |
|
No worries! We carry this out-of-tree so it can wait until the next patch release. Just wanted to make sure there wasn't a deeper issue. |
|
@bbockelm In order for us to be able to merge this, please rebase it on top of the current master branch. If CI is red I cannot merge. Thanks. |
80516ee to
7d9cc36
Compare
When using `X-Transfer-Status`, there were three subtle HTTP protocol errors: - The trailer contained a `\n` which is not a permitted character. This caused clients to view the last CRLF as leftover data on an idle TCP channel. - A trailer was sent but no `Trailer` header was sent in the response. - When the server said that the connection would be kept alive but an error occurred, it closed the TCP connection unexpectedly on the client. The server now keeps the TCP connection open if keepalive is enabled, provided the HTTP response was sent cleanly.
This unit test (including an XrdOss wrapper to inject failures) tests the behavior of the HTTP server when a read error occurs. The server should either abruptly close the TCP connection or, if Trailers are enabled, send an error message and keep the connection alive.
6dae5dd to
64847a8
Compare
|
@amadio - fixed the underlying issue. This PR introduces a small OSS for injecting failures in the unit tests. Some of the recent CMake changes caused the location of the built library to change and hence the generated test configs were incorrect. Easy fix - but I suppose it's one of the dangers of having it sit out on a branch for so long. |
|
Libraries in configuration files should not use paths, just the library name. XRootD should always load the right library from the same place as the other ones (in this case, from the |
When using
X-Transfer-Status, there were three subtle HTTP protocolerrors:
\nwhich is not a permitted character. This caused clients to view the last CRLF as leftover data onan idle TCP channel.
Trailerheader was sent in the response.To prevent regressions, this adds the first unit test that injects read failures mid-transfer.