MingwX64 support ?

[martinbonnin]

What would be needed to get mingwX64 support? adding the target? updating CI and publication? Anything else?

[swankjesse]

That might be it. Send a PR?

[martinbonnin]

👍 I added the target and CI build but it chokes on tests: https://github.com/martinbonnin/okio/runs/929628421?check_suite_focus=true. Will keep looking.

[martinbonnin]

Small update: there are at least three different flavors of errors:

1. Encoding errors: looks like something in CI is using CP1252 encoding somehow. This triggers error when compiling Java (ByteStringTest.java:276: error: unmappable character for encoding Cp1252) and also during tests (java.lang.AssertionError at BufferedSinkTest.java:183) (fixed with martinbonnin@d55fe5a)
2. Timing errors: Object.wait() can return slightly before the timeout. I just had a case where it returned after ellapsedNanos=996269000 (should have been >=1000000000). Looking at Object.wait, it's not 100% clear whether there's a guarantee on the timeout or if it's approximate.
3. Kotlin compiler error: Could not find serialized descriptor for index. Might be related to Could not find serialized descriptor for index JetBrains/kotlin-native#2933 (seems to work with Kotlin 1.4 but other tests fail: https://github.com/martinbonnin/okio/runs/931784172?check_suite_focus=true) Fixed with 1.4

[swankjesse]

Thanks for digging into this!

For the timing thing it should be fine to relax the tests. I assume the underlying wait mechanisms aren’t precise enough.

[martinbonnin]

Kotlin 1.4 fixed the compiler error 🎉 . The only thing left is the timing error.

Relaxing the tests is most likely not enough, or we would have to relax them a lot for the PipeTests to pass. The main problem is that Timeout.waitUntilNotified() does not detect reliably if the timeout was reached, which means that when reading a pipe, we might end up looping several times, even after the timeout is reached, waiting e.g. 3s instead of 1s (but can be worse, or maybe even loop forever).

[volo-droid]

2. Timing errors: Object.wait() can return slightly before the timeout. I just had a case where it returned after ellapsedNanos=996269000 (should have been >=1000000000). Looking at Object.wait, it's not 100% clear whether there's a guarantee on the timeout or if it's approximate.

@martinbonnin There's no guarantee on Object.wait() timeout because of spurious wakeups:

A thread can also wake up without being notified, interrupted, or timing out, a so-called spurious wakeup.

[martinbonnin]

There's no guarantee on Object.wait() timeout because of spurious wakeups

Indeed 👍 . It's weird that on Windows, this always happens a few nanoseconds before the timeout, which makes it look more like inaccurate timers than spurious wakeups but in the end it's the same result.

What this all means is that the Timeout.waitUntilNotified() contract cannot hold and that some of its code should be moved to the caller site, i.e:

Replace

      synchronized(buffer) {
        check(!sourceClosed) { "closed" }
        if (canceled) throw IOException("canceled")

        while (buffer.size == 0L) {
          if (sinkClosed) return -1L
          timeout.waitUntilNotified(buffer) // Wait until the sink fills the buffer.
          if (canceled) throw IOException("canceled")
        }

        val result = buffer.read(sink, byteCount)
        (buffer as Object).notifyAll() // Notify the sink that it can resume writing.
        return result
      }

with something like:

      synchronized(buffer) {
        check(!sourceClosed) { "closed" }
        if (canceled) throw IOException("canceled")

        val start = timeout.nanoTime() 
        while (buffer.size == 0L) {
          val remainingTime = timeout.remainingTime(start)
          if (remainingTime <= 0) {
            throw InterruptedIOException("timeout")
          }
          if (sinkClosed) return -1L
          buffer.wait(remainingTime)
          if (canceled) throw IOException("canceled")
        }

        val result = buffer.read(sink, byteCount)
        (buffer as Object).notifyAll() // Notify the sink that it can resume writing.
        return result
      }

@swankjesse any thoughts about adding Timeout.nanoTime(): Long and Timeout.remainingTime(Long): Long?

If that works for you, I could modify the places needed to make the windows tests pass (Pipe.kt, maybe a few others) and leave the other untouched?

[swankjesse]

No, I don't like that. Timeout.waitUntilNotified() needs to resume when it is notified. I think these changes would break that.

If we wanted to change the API to mitigate spurious wakeups, it could take a lambda with a condition.

timeout.waitUntil { buffer.size != 0 }

[swankjesse]

Also - there's a difference between spurious wakeups and bad timer resolution.

Spurious wakeups haven't been a problem in my experience. The effect I'm aware of is notify() wakes up multiple waiters instead of exactly one.

Bad timer resolution is waking up a few ns early. I don't think this is problematic in practice either. If you look at the source code of OpenJDK’s Object.wait(long, int) you'll be horrified as I was that the extra precision is accepted but not used. The consequence is that timeouts aren't very good if the timeout is <= 1ms. This is a bug in the JVM and perhaps we should report it to their maintainers.

[swankjesse]

http://hg.openjdk.java.net/jdk7/jdk7/jdk/file/tip/src/share/classes/java/lang/Object.java#l447

[martinbonnin]

http://hg.openjdk.java.net/jdk7/jdk7/jdk/file/tip/src/share/classes/java/lang/Object.java#l447

🙈

Timeout.waitUntilNotified() needs to resume when it is notified. I think these changes would break that.

To clarify, the proposal is to not use Timeout.waitUntilNotified but wait at the call site instead. Timeout.waitUntilNotified would most likely be untouched and work as before.

timeout.waitUntil { buffer.size != 0 }

This API would need to have the object to wait on passed as a param so maybe something like timeout.waitUntil(buffer) { buffer.size != 0 } ?

[swankjesse]

This API would need to have the object to wait on passed as a param so maybe something like timeout.waitUntil(buffer) { buffer.size != 0 } ?

Yup, that.