Skip to content

Update README.md for async/await #554

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

Merged
merged 3 commits into from
Feb 9, 2022

Conversation

dnadoba
Copy link
Collaborator

@dnadoba dnadoba commented Feb 7, 2022

We want to highlight the new async/await API in the README.

  • add new examples for async/await but keeps old examples for SwiftNIOs EventLoopFutures
  • change URL used in examples from swift.org to apple.com because swift.org doesn't respect that we don't support compression by default which results in gibberish output if examples are run by users.
  • increase recommended Xcode Version to 13.2 and Swift Version to 5.5.2
  • increase recommend AsyncHTTPClient version to 1.9.0

README.md Outdated
let progress = Double(receivedBytes)/Double(expectedBytes)
print("progress: \(Int(progress * 100))%")
}
// in case backpressure is needed, all reads will be paused until the end of the for loop.
Copy link
Member

Choose a reason for hiding this comment

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

backpressure has nothing todo with the loop, and I don't think we should explain it here. It just works ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should mentioned this at least somewhere. AFAIK, this is not documented anywhere or am I missing something? This is a really great feature and we should definitely mention it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm with @dnadoba that we should mention it, but I'm with @fabianfett that I don't think we need to explain it: it just works. In this case, maybe at the top of the loop I'd change that comment to something like:

// asynchronously iterates over all body fragments
// this loop will automatically propagate backpressure correctly

README.md Outdated
}
// in case backpressure is needed, all reads will be paused until the end of the for loop.
}
print("did receive \(receivedBytes) bytes")
Copy link
Member

Choose a reason for hiding this comment

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

close AHC?

Copy link
Collaborator Author

@dnadoba dnadoba Feb 8, 2022

Choose a reason for hiding this comment

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

The full example has the shutdown():

let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
do {
let request = HTTPClientRequest(url: "https://apple.com")
let response = try await httpClient.execute(request, timeout: .seconds(30))
print("HTTP head", response)
// if defined, the content-length headers announces the size of the body
let expectedBytes = response.headers.first(name: "content-length").flatMap(Int.init)
var receivedBytes = 0
// Asynchronously iterates over all body fragments.
for try await buffer in response.body {
// For this example, we are just interested in the size of the fragment
receivedBytes += buffer.readableBytes
if let expectedBytes = expectedBytes {
// if the body size is known, we calculate a progress indicator
let progress = Double(receivedBytes)/Double(expectedBytes)
print("progress: \(Int(progress * 100))%")
}
// in case backpressure is needed, all reads will be paused until returned future is resolved
}
print("did receive \(receivedBytes) bytes")
} catch {
print("request failed:", error)
}
// it is important to shutdown the httpClient after all requests are done, even if one failed
try await httpClient.shutdown()

We would need to nest it in a do catch to be correct but I'm fine with it if you want to add it. I didn't add it because the old one also has no shutdown. However, I'm just discovering that the old examples doesn't create an AsyncHTTPClient, it just assumes that there is one. I see two options:

  • remove HTTPClient creation
  • or add shutdown

WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think add the shutdown and nest in a do catch.

@dnadoba dnadoba added the 🆕 semver/minor Adds new public API. label Feb 8, 2022
// handle body
} else {
// handle remote error
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So much nicer! 🎉

README.md Outdated
let progress = Double(receivedBytes)/Double(expectedBytes)
print("progress: \(Int(progress * 100))%")
}
// in case backpressure is needed, all reads will be paused until the end of the for loop.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm with @dnadoba that we should mention it, but I'm with @fabianfett that I don't think we need to explain it: it just works. In this case, maybe at the top of the loop I'd change that comment to something like:

// asynchronously iterates over all body fragments
// this loop will automatically propagate backpressure correctly

README.md Outdated
}
// in case backpressure is needed, all reads will be paused until the end of the for loop.
}
print("did receive \(receivedBytes) bytes")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think add the shutdown and nest in a do catch.

@dnadoba dnadoba force-pushed the dn-async-await-readme branch from d82ff28 to 6e974af Compare February 8, 2022 12:17
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Awesome!

@fabianfett fabianfett requested a review from glbrntt February 9, 2022 14:46
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks good, I left two tiny nits though

README.md Outdated

if let expectedBytes = expectedBytes {
// if the body size is known, we calculate a progress indicator
let progress = Double(receivedBytes)/Double(expectedBytes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, space makes it more readable:

Suggested change
let progress = Double(receivedBytes)/Double(expectedBytes)
let progress = Double(receivedBytes) / Double(expectedBytes)

@dnadoba dnadoba merged commit c4a63c1 into swift-server:main Feb 9, 2022
@dnadoba dnadoba deleted the dn-async-await-readme branch February 10, 2022 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants