Add logic to Platform type for AWS configuration use cases

[kggilmer]

Issue #

awslabs/aws-sdk-kotlin#216

Companion PR: awslabs/aws-sdk-kotlin#315

Description of changes

Add additional functions and properties to the Platform type for the purposes of loading AWS configuration files.

• The file loading API is written to handle AWS config file loading only. The spec states that non-existent files should be treated as if they are empty properties. No File I/O errors are used. If in the future we need tighter behavior on file loading this may need to be updated. For now though I favor YAGNI and KISS for file loading.

Scope

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[aajtodd]

It seems like some of these are very platform specific in that they may or may not be supported by a platform (e.g. getProperty() or readFile(), the former is clearly JVM specific, the latter isn't supported on browser/wasm, etc).

I'm not sure this was the intent of Platform type originally. It was more an expect/actual for things that are likely available on all platforms or have reasonable defaults (like returning null for getenv() if environment variables aren't supported). The concept of getProperty() is only applicable to JVM though. And readFile() doesn't really have a sensible default.

I understand why these were added here, I'm not sure I have a more solid suggestion right now but I'm also not sure I love continuing down this path.

[aajtodd]

nit

filePathSeparator seems more appropriate to me

[kggilmer]

sure, updated

[aajtodd]

correctness

1. Should this be suspend? (probably...note you can get an SdkByteReadChannel from a file or path already)
2. Should we really return null on error? This seems likely to hide issues that could be difficult to track down.

[kggilmer]

I don't have any use cases other than the functionality covered by this PR for reading files. As I mention in the notes above based on this I made this to support my use case and nothing else in the spirit of YAGNI.

What value does adding suspend do here? In the JVM implementation we only call File.readText() which does not suspend. I would be surprised if there is suspend equivalents from JS or native but maybe I'm missing something?

I return null on errors because that maps directly to the requirements of the spec. (Inaccessible File: If a file is not found or cannot be opened in the configured location, the implementation must treat it as an empty file, and must not attempt to fall back to any other location.) If you'd like some other behavior I'd like to see specific use cases. I'm not saying returning null is the right answer here, but I don't want to come up w/ something on a whim. It seems that when this second use case for file loading comes around we'll need to re-address this and update as needed. Or, if this turns out to be the only place we need to read files then it's probably fine as is.

[aajtodd]

I don't have any use cases other than the functionality covered by this PR for reading files. As I mention in the notes above based on this I made this to support my use case and nothing else in the spirit of YAGNI.

I think that adding single specific use cases to a general purpose utility is perhaps not the right way to proceed. If semantics are baked in to apply to a single use case then the next time we need to read a file for any reason and we update this code because it wasn't made general purpose we may break that single use case.

What value does adding suspend do here? In the JVM implementation we only call File.readText() which does not suspend. I would be surprised if there is suspend equivalents from JS or native but maybe I'm missing something?

Where is this going to be called from? If the answer is anywhere from an operation then we've just broken the concurrency model OR we move the burden of the blocking call into the call site. Coroutines aren't supposed to block and this introduces a blocking IO call. My suggestion was to remove File.readText() in favor of a suspending equivalent OR at the very least run it on the appropriate dispatcher for the caller:

// JVM implementation

actual suspend fun readFileOrNull(path: String): ByteArray? = try {
         with(Dispatchers.IO) {
             File.readBytes(path)
         }
    }catch (ex: IOException) {
        null
     }

This is better to me for a few reasons:

• readFileOrNull() name matches semantics elsewhere orNull is used which usually implies errors are swallowed/ignored
• it returns a ByteArray? rather than String? which makes this more general purpose. Now we can read binary files if needed. Callers that want text only have to call decodeToString()
• It is usable from a coroutine and callers don't need to think about the blocking call or misusing it accidentally

Leave it as is for now if you feel it's right but I personally hate adding a public API without thinking through how it will/could be used. YAGNI doesn't really apply here IMHO.

[kggilmer]

If semantics are baked in to apply to a single use case then the next time we need to read a file for any reason and we update this code because it wasn't made general purpose we may break that single use case.

I think this is the general nature of development. Also, this code is internal.

remove File.readText() in favor of a suspending equivalent

I'm not aware of a JVM suspending equivalent. I notice something about Java NIO w/ channels but seems overly complex and I can't think of a concrete benefit. LMK if you know of something here.

Otherwise, your suggestions and code snippets are good and thank you for raising the concurrency concerns regarding blocking calls in coroutines. I'll update the PR to use your suggestion.

[aajtodd]

I'm not aware of a JVM suspending equivalent.

FWIW we have wrappers in our runtime to create SdkByteReadChannels.

The suggestion I made is fine and more or less equivalent though (file channels are moved to Dispatchers.IO internally as well for all the blocking reads).

[kggilmer]

I understand why these were added here, I'm not sure I have a more solid suggestion right now but I'm also not sure I love continuing down this path.

I don't have any better ideas for where to put this necessary behavior, but let me know if you have any. Platform to me seems like a good, obvious abstraction for platform specific functions. I get that not all platforms support all functionality but I think we've been in that boat from the beginning. From my understanding CRT also works in this way when dealing w/ some feature that is not available on some platform.