improve API usages around String/Data/ByteBuffer

[weissi]

• fix String(data:encoding) to the better String(decoding:as:)
• fix String.data(encoding:) to the better Data(string.utf8)
• fix ByteBuffer conversion
• fix deinit abuse

[adam-fowler]

These all look good

[adam-fowler]

@weissi Actually the HTTPClient code will need backed out. We need to provide support for a http client with proxy settings so cannot use the shared client.

[weissi]

@weissi Actually the HTTPClient code will need backed out. We need to provide support for a http client with proxy settings so cannot use the shared client.

Shouldn't HTTPClient.shared just understand and enable the standard http_proxy etc environment variables? If we back this out, we'd need to fix the lifecycle here really.

Alternatively, once proxy support is ready you could create your own singleton in Swiftly.

[adam-fowler]

Shouldn't HTTPClient.shared just understand and enable the standard http_proxy etc environment variables?

That'd be nice. I don't think it does at the moment.

[patrickfreed]

If we go the shared route, I don't think we need this class at all actually. The original intent was to be able to share a single HTTP client via class's arc semantics and also to ensure it shut down when the application was shutting down, both of which seem to be solved by HTTPClient.shared.

If we don't go this route, then we probably still can get rid of this assuming we don't need to call syncShutdown ourselves, which it seems like we don't need to?

[weissi]

If we go the shared route, I don't think we need this class at all actually. The original intent was to be able to share a single HTTP client via class's arc semantics and also to ensure it shut down when the application was shutting down, both of which seem to be solved by HTTPClient.shared.

Right, this doesn't really work because you can't know what thread will call deinit, so if you call syncShutdown in there that can be pretty bad.

There are really two 'correct' ways:

1. in main call: let client = HTTPClient(...); defer { try! client.syncShutdown() } and dependency inject it
2. use a singleton. Either HTTPClient.shared or your own singleton

Most server people prefer (1) but Apple's SDKs are typically singleton-heavy and therefore the Swift community is often quite keen on simplicity which would be (2). Especially for a command-line utility like Swiftly, having a HTTPClient-singleton isn't really that bad.

Of course it's a serious design issue in HTTPClient that some of the configuration (like the proxy settings) can only be configured using the top-level object (HTTPClient) that you're really meant to share everywhere (swift-server/async-http-client#392).

[patrickfreed]

Yeah, I think a singleton would be fine for swiftly, it's basically what we have already. Until shared can be configured to handle the proxy settings, it seems like we'll need to do it ourselves though. I think we can do that in a separate commit / PR or here if you'd like, either is fine with me.

[weissi]

Shouldn't HTTPClient.shared just understand and enable the standard http_proxy etc environment variables?

That'd be nice. I don't think it does at the moment.

No, but it should and it shouldn't be hard to implement I think.

[adam-fowler]

I'm good with this going in just now. We can resolve the proxy thing separately

[weissi]

I'm good with this going in just now. We can resolve the proxy thing separately

Thanks. Why don't we do that so we can move forward.

[patrickfreed]

sgtm

[patrickfreed]

thanks for the contribution!