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

[URLSession] Fix handling of compressed responses. #2108

Merged

Conversation

drodriguez
Copy link
Contributor

When the server answers with a compressed response (with
Content-Encoding header), cURL is configured to automatically decompress
the response. However, there was some inconsistencies with the
Foundation implementation in Darwin platforms, and some bugs in the
handling of compressed responses.

  • In Darwin Foundation, the expected number of bytes is -1 for
    compressed responses. This is because Content-Lenght will report the
    length of the compressed content, but the total bytes written reports
    uncompressed lengths. The code is changed to set the expected number
    of bytes to -1 if the response includes a Content-Encoding header
    different to "identity".
  • Since the expected number of bytes is unknown, the data received
    callback cannot check for the total bytes received to be equal to
    close the file handler and provide the temporary file URL to the upper
    level. That responsability has been moved into the complete task
    callback, where it was already happening for tasks with completion
    blocks.
  • Added two tests (one for data tasks, one for download tasks) with
    gzipped data.
  • Since gzipped data cannot be represented by UTF-8 strings, the
    HTTPServer code has to be modified to allow providing raw data as part
    of the HTTP response. There's a lot of changes so the body is raw
    data, and the previously provided strings are transformed into data
    using UTF-8 instead.
  • There was a small bug in the HTTPServer code where the setUp will wait
    for a flag to be different of -2 to indicate the server is ready.
    However, the flag should be checked against -1, which is the initial
    state, while -2 is the final state. I found this when the server port
    that my test wanted to use was uninitialized, because the server is
    started asynchronously in another queue, and the value wasn't valid
    yet.

@spevans
Copy link
Contributor

spevans commented Apr 14, 2019

@swift-ci test

@ianpartridge ianpartridge requested a review from pushkarnk April 14, 2019 12:20
@drodriguez drodriguez force-pushed the fix-urlsession-gzipped-responses branch from 7f945b0 to 979ce02 Compare April 14, 2019 17:53
@drodriguez
Copy link
Contributor Author

Changed an incorrect test assert message.

@spevans
Copy link
Contributor

spevans commented Apr 14, 2019

@swift-ci test

1 similar comment
@pushkarnk
Copy link
Member

@swift-ci test

Copy link
Member

@pushkarnk pushkarnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff has two-space indentation. Could you please change it to four-space, something which we've been following here? The patch looks great otherwise.

When the server answers with a compressed response (with
Content-Encoding header), cURL is configured to automatically decompress
the response. However, there was some inconsistencies with the
Foundation implementation in Darwin platforms, and some bugs in the
handling of compressed responses.

- In Darwin Foundation, the expected number of bytes is -1 for
  compressed responses. This is because Content-Lenght will report the
  length of the compressed content, but the total bytes written reports
  uncompressed lengths. The code is changed to set the expected number
  of bytes to -1 if the response includes a Content-Encoding header
  different to "identity".
- Since the expected number of bytes is unknown, the data received
  callback cannot check for the total bytes received to be equal to
  close the file handler and provide the temporary file URL to the upper
  level. That responsability has been moved into the complete task
  callback, where it was already happening for tasks with completion
  blocks.
- Added two tests (one for data tasks, one for download tasks) with
  gzipped data.
- Since gzipped data cannot be represented by UTF-8 strings, the
  HTTPServer code has to be modified to allow providing raw data as part
  of the HTTP response. There's a lot of changes so the body is raw
  data, and the previously provided strings are transformed into data
  using UTF-8 instead.
- There was a small bug in the HTTPServer code where the setUp will wait
  for a flag to be different of -2 to indicate the server is ready.
  However, the flag should be checked against -1, which is the initial
  state, while -2 is the final state. I found this when the server port
  that my test wanted to use was uninitialized, because the server is
  started asynchronously in another queue, and the value wasn't valid
  yet.
@drodriguez drodriguez force-pushed the fix-urlsession-gzipped-responses branch from 979ce02 to 770a8f5 Compare April 15, 2019 18:13
@drodriguez
Copy link
Contributor Author

Fixed the indentation. I hope I fixed every case, but don’t hesitate to point out if you find any more.

@spevans
Copy link
Contributor

spevans commented Apr 15, 2019

@swift-ci test

1 similar comment
@pushkarnk
Copy link
Member

@swift-ci test

@pushkarnk
Copy link
Member

@drodriguez Thanks! Looks good to me now.

@spevans
Copy link
Contributor

spevans commented Apr 16, 2019

@swift-ci test and merge

1 similar comment
@pushkarnk
Copy link
Member

@swift-ci test and merge

@swift-ci swift-ci merged commit 5f48836 into swiftlang:master Apr 16, 2019
@drodriguez drodriguez deleted the fix-urlsession-gzipped-responses branch July 16, 2019 22:41
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.

4 participants