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

Provide an http client example. #32

Merged
merged 14 commits into from Mar 13, 2019

Conversation

Projects
None yet
4 participants
@mike-neck
Copy link
Contributor

mike-neck commented Sep 1, 2018

Provide an HTTPS example. (#12)

Motivation:

To provide an example for people to use swift-nio-ssl at their projects with ease.

Modifications:

  • Add NIOHTTP1Client module.

Result:

A good example for getting started with swift-nio-ssl.

@swift-nio-bot

This comment has been minimized.

Copy link

swift-nio-bot commented Sep 1, 2018

Can one of the admins verify this patch?

@mike-neck mike-neck force-pushed the mike-neck:add_http_client_example branch from bffbf42 to f360626 Sep 2, 2018

@weissi
Copy link
Member

weissi left a comment

Thanks @mike-neck that's a great start, we need a few changes here but then this can go in.

let bootstrap = ClientBootstrap(group: eventLoopGroup)
.channelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
.channelInitializer { channel in
_ = channel.pipeline.add(handler: openSslHandler)

This comment has been minimized.

@weissi

weissi Sep 13, 2018

Member
  1. the openSslHandler should be constructed inside of the channelInitializer closure as it might be run more than once (see the warning in here )
  2. the futures should be properly chained:
[...]
.channelInitializer { channel in
    return channel.pipeline.add(handler: OpenSSLClientHandler(...)).then {
        channel.pipeline.addHTTPClientHandlers()
    }.then {
        channel.pipeline.add(handler: HTTPResponseHandler(promise))
    }
}

This comment has been minimized.

@mike-neck

mike-neck Sep 14, 2018

Author Contributor

Thanks! I fixed them. Please see 6e21fc7 .

("User-Agent", "swift-nio"),
("Accept", "application/json")
])
_ = channel.write(HTTPClientRequestPart.head(request))

This comment has been minimized.

@weissi

weissi Sep 13, 2018

Member
  1. please use

channel.write(..., promise:nil)

if you don't care about the result
2. the sendRequest function should probably return an EventLoopFuture<Void> which is the result of the writeAndFlush

This comment has been minimized.

@mike-neck

mike-neck Sep 14, 2018

Author Contributor

I made sendRequest to return EventLoopFuture<Void> and cascade promise(via your advice on #32 (comment)). Please see 09809a4.

}

bootstrap.connect(host: "httpbin.org", port: 443)
.whenSuccess { sendRequest($0) }

This comment has been minimized.

@weissi

weissi Sep 13, 2018

Member

after changing sendRequest to return an EventLoopFuture you should use

bootstrap.connect(...)
    .then {
        sendRequest($0)
    }
    .cascadeFailure(promise: promise)

the reason being that your program will hang until promise is fulfilled. The problem is if we can never write the request in sendRequest you'll also never get a response so promise won't ever be fulfilled, ie. we'll hang forever. Therefore in case of an error in sendRequest we want to cascade that failure into promise so that the program terminates.

This comment has been minimized.

@mike-neck

mike-neck Sep 14, 2018

Author Contributor

I fixed it in the commit 09809a4.

_ = channel.writeAndFlush(HTTPClientRequestPart.end(nil))
}

bootstrap.connect(host: "httpbin.org", port: 443)

This comment has been minimized.

@weissi

weissi Sep 13, 2018

Member

we should probably accept hostname, port and target (possibly as a URL) from the command line.

This comment has been minimized.

@mike-neck

mike-neck Sep 14, 2018

Author Contributor

I had never thought of receiving parameters.
I made it to accept command line argument. Please see 8a7a9ac.

And I added README for this example. ffb7984

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Sep 13, 2018

@mike-neck sorry for the massive delay, I totally missed your PR here.

@weissi
Copy link
Member

weissi left a comment

thanks! Almost there

print(responseBody)
}
case .end(_):
_ = ctx.channel.close()

This comment has been minimized.

@weissi

weissi Sep 17, 2018

Member

ctx.channel.close(promise: nil) expresses what you want a little bit better and doesn't allocate

This comment has been minimized.

@mike-neck

mike-neck Sep 18, 2018

Author Contributor

I changed in 10c09d5.


func errorCaught(ctx: ChannelHandlerContext, error: Error) {
print("Error: ", error)
_ = ctx.channel.close()

This comment has been minimized.

@weissi

weissi Sep 17, 2018

Member

same here, ctx.channel.close(promise: nil)

This comment has been minimized.

@mike-neck

mike-neck Sep 18, 2018

Author Contributor

I changed in 10c09d5.

request.headers = HTTPHeaders([
("Host", url.host!),
("User-Agent", "swift-nio"),
("Accept", "application/json")

This comment has been minimized.

@weissi

weissi Sep 17, 2018

Member

I think you should add ("Connection", "close") as you're closing the connection after receiving the response.

This comment has been minimized.

@mike-neck

mike-neck Sep 18, 2018

Author Contributor

Right. But if I add this header, HTTPResponseDecoder seems to signal fireChannelInactive earlier than fireChannelRead even though it has pending inputs.

This comment has been minimized.

@mike-neck

mike-neck Sep 18, 2018

Author Contributor

I changed in 10c09d5 to use Connection header, but currently it seems that this handler's channelRead is not called.

This comment has been minimized.

@Lukasa

Lukasa Sep 18, 2018

Contributor

Are you sure? Please try updating to the current master, because when I run this patch against current master it seems that channelRead is called fine.

This comment has been minimized.

@Lukasa

Lukasa Sep 18, 2018

Contributor

Aha, the issue appears to be intermittent.

This comment has been minimized.

@Lukasa

Lukasa Sep 18, 2018

Contributor

Ok, there's a straightforward logic bug in this module. I'll fix it up.

This comment has been minimized.

@Lukasa

Lukasa Sep 18, 2018

Contributor

See #40.

This comment has been minimized.

@weissi

weissi Sep 18, 2018

Member

awesome job @mike-neck and @Lukasa for finding this issue and fixing it!

This comment has been minimized.

@mike-neck

mike-neck Sep 18, 2018

Author Contributor

@Lukasa Thanks it's working fine!

print("\(name): \(value)")
}
case .body(var byteBuffer):
if let responseBody = byteBuffer.readString(length: byteBuffer.readableBytes) {

This comment has been minimized.

@weissi

weissi Sep 17, 2018

Member

this can fail with UTF-8 strings (let's assume the server sends "🎉" in chunks of one byte, that would be f0, 9f, 8e, 89 then this would print five times instead of 🎉.)

but I might be fine with that, CC @Lukasa

This comment has been minimized.

@Lukasa

Lukasa Sep 17, 2018

Contributor

Yeah, maybe we should skip print and instead write bytes to stdout?

This comment has been minimized.

@weissi

weissi Sep 17, 2018

Member

sounds like a plan to me.

This comment has been minimized.

@mike-neck

mike-neck Sep 18, 2018

Author Contributor

Thanks! I removed print and use stdout in 39b519a.

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Feb 25, 2019

@mike-neck so sorry, I totally didn't see the github notifications. If you wouldn't mind fixing the conflicts and making it work for NIO 2 (mostly renames), then we should get this merged.

mike-neck added some commits Sep 1, 2018

Add "/.idea" to the project's ignore file.
Motivation:

To develop with AppCode.

Modifications:

Add "/.idea" to the project's ignore file.

Result:

Add "/.idea" to the project's ignore file.
Provide an HTTPS example. (#12)
Motivation:

To provide an example for people to use swift-nio-ssl at their projects with ease.

Modifications:

- Add NIOHTTP1Client module.

Result:

A good example for getting started with swift-nio-ssl.
Make channel closed after receiving HTTPClientResponsePart.end. Becau…
…se if large response is returned, channelReadComplete is called twice or more times.
@mike-neck

This comment has been minimized.

Copy link
Contributor Author

mike-neck commented Mar 4, 2019

It may become long(2 or 3 days) to rebase this branch and to make it work for NIO 2, because there are many changes in repository.

@mike-neck mike-neck force-pushed the mike-neck:add_http_client_example branch from 39b519a to f7ebe32 Mar 12, 2019

@mike-neck

This comment has been minimized.

Copy link
Contributor Author

mike-neck commented Mar 12, 2019

@weissi
I fixed compile error on NIO2. But this code doesn't work, because NIOSSLClientHandler closes a connection before channelRead of HTTPResponseDecoder is called.

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Mar 12, 2019

@mike-neck thanks very much for updating! I assume you've been testing on macOS? If yes, you're probably blocked on #72. At this moment, on macOS we can't find the trust roots so cert validation will fail...

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Mar 12, 2019

@mike-neck yes, I can confirm: I just tested on Linux and everything was fine after applying the diff below. You forgot to rename some ctx: ChannelHandlerContext to context: ChannelHandlerContext but that's all (aside from #72 not being merged yet)

diff --git a/Sources/NIOHTTP1Client/main.swift b/Sources/NIOHTTP1Client/main.swift
index ac39867..dd87ebb 100644
--- a/Sources/NIOHTTP1Client/main.swift
+++ b/Sources/NIOHTTP1Client/main.swift
@@ -30,7 +30,7 @@ private final class HTTPResponseHandler: ChannelInboundHandler {
 
     typealias InboundIn = HTTPClientResponsePart
 
-    func channelRead(ctx: ChannelHandlerContext, data: NIOAny) {
+    func channelRead(context: ChannelHandlerContext, data: NIOAny) {
         let httpResponsePart = unwrapInboundIn(data)
         switch httpResponsePart {
         case .head(let httpResponseHeader):
@@ -43,21 +43,21 @@ private final class HTTPResponseHandler: ChannelInboundHandler {
                 FileHandle.standardOutput.write(data)
             }
         case .end(_):
-            closeFuture = ctx.channel.close()
+            closeFuture = context.channel.close()
             promise.succeed(())
         }
     }
 
-    func channelInactive(ctx: ChannelHandlerContext) {
+    func channelInactive(context: ChannelHandlerContext) {
         if closeFuture == nil {
-            closeFuture = ctx.channel.close()
+            closeFuture = context.channel.close()
             promise.fail(ChannelError.inputClosed)
         }
     }
 
-    func errorCaught(ctx: ChannelHandlerContext, error: Error) {
+    func errorCaught(context: ChannelHandlerContext, error: Error) {
         print("Error: ", error)
-        closeFuture = ctx.channel.close()
+        closeFuture = context.channel.close()
         promise.succeed(())
     }
 }
@mike-neck

This comment has been minimized.

Copy link
Contributor Author

mike-neck commented Mar 13, 2019

@weissi Yes, I had run this on macOS. And I'll try this on ubuntu(Docker). (Since it takes too long for my machine to download Swift5, it will become tomorrow to accomplish it.)

mike-neck and others added some commits Mar 13, 2019

@weissi

weissi approved these changes Mar 13, 2019

Copy link
Member

weissi left a comment

awesome, thanks @mike-neck

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Mar 13, 2019

@swift-nio-bot test this please

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Mar 13, 2019

@swift-nio-bot test this please

@weissi weissi added this to the 2.0.0 milestone Mar 13, 2019

@weissi weissi merged commit 5b60325 into apple:master Mar 13, 2019

1 check passed

pull request validation (5.0) Build finished.
Details
@mike-neck

This comment has been minimized.

Copy link
Contributor Author

mike-neck commented Mar 13, 2019

@weissi Thanks I confirmed this works on ubuntu(Docker).

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Mar 13, 2019

@mike-neck thanks! it should also now work on macOS (and did on my machine)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.