cats-effect

cats-effect copied to clipboard

Addresses #3313.

Thank you for looking at this! An interesting question for us to answer though is what we want to do with Native and JavaScript. This is a bit different from Env, which works on all three platforms (with the exception of in-browser JS).

Native and JavaScript

They both support System properties. It's basically just a global mutable map.

A question: the getAndSet, modify, and similar methods look like the ones on Ref. However, here they are not atomic, right? So having them could be somewhat misleading. (While having only get and set would make it clear, that there is no atomicity for modification.)

I would agree. TBH, I wondered whether we should try and make them atomic, but that's probably not something you'd expect by system props.

I don't think we can make them atomic. Or do you know a way?

Right, not sure how could be made atomic. Citing Arman, it's basically just a global mutable map. I'll remove getAndSet and getAndUpdate but I think that update is still useful if we make clear that there are no guarantees about concurrent modifications. It's a handy wrapper for a get followed by set. I also like modify as a wrapper but would like to hear other opinions.

I think that update is still useful if we make clear that there are no guarantees about concurrent modifications

Unfortunately I think it's still confusing. Anyway, I expect 99% of usecases to just be reading system properties. APIs that require setting/updating system properties seem really dicey 🙄

I'd just go with get, set and remove. If that's the functionality offered by the platform, and we can't provide atomicity, its best simply to wrap those.

Similar to Env, we can also add something to list/iterate the keys/entries.

[BIKESHEDDING] I'm late to the party to comment probably, but I fear that Prop[F] as a name may clash with scalacheck or one of its integrations.

but I fear that Prop[F] as a name may clash with scalacheck or one of its integrations.

I actually also dislike the name Prop[F], without even considering the Scalacheck issues (good point).

The reason that I dislike it is because this API is very specifically abstracting over the (global singleton) system properties, not the generic JDK Properties datatype.

If we want to keep the name short we could rename to SysProps[F] perhaps. Or just go for SystemProperties[F] which I think is my preference.

I think the entries is not an atomic snapshot of the system properties. it would be nice if we could do that, but I don't think it's possible with the available JDK API.

The JDK suggests there is some notion of "bulk" operations.

    /**
     * Properties does not store values in its inherited Hashtable, but instead
     * in an internal ConcurrentHashMap.  Synchronization is omitted from
     * simple read operations.  Writes and bulk operations remain synchronized,
     * as in Hashtable.
     */

https://github.com/openjdk/jdk/blob/388fcf03c02c41bb690733e8565642c24ead56e0/src/java.base/share/classes/java/util/Properties.java#L161-L167

I am still trying to figure out what the bulk operations are exactly. If serializing the properties to a stream is one such bulk operation, perhaps we can use that to make an atomic copy first ... 🤔

I think the entries is not an atomic snapshot of the system properties. it would be nice if we could do that, but I don't think it's possible with the available JDK API.

It is possible, I think I was able to fix it. Thanks for catching :)

Or just go for SystemProperties[F] which I think is my preference.

That's my favorite too.