Calling `retain` on a closed ArrowBuf revives it, into an undefined state

[jarohen]

Describe the bug, including details regarding any error messages, version, and platform.

@Test
fun testArrowBufRetainBug() {
    RootAllocator().use { al ->
        val buf = al.buffer(10)
        buf.close()

        // correctly throws
        assertThrows<IllegalArgumentException> { buf.referenceManager.retain() }

        assertEquals(0, buf.refCnt()) // nope, it's 1

        // doesn't throw, ref-count is 1 - so the caller assumes they've successfully taken a reference
        // but the underlying memory has already been reclaimed and re-used
        assertThrows<IllegalArgumentException> { buf.referenceManager.retain() }
    }
}

Caused by the getAndAdd in BufferLedger.retain(int) - this is what leaves the ref-count positive, so on the next call, this doesn't fail.

Some kind of compareAndSet instead, perhaps?

Cheers,

James

[lidavidm]

Hmm, I'd argue that after the first exception the buffer is already in an undefined state. compareAndSet would require a CAS loop - might be a pessimization for the common case? We could reset back to 0 before throwing instead?

[jarohen]

I'd argue that after the first exception the buffer is already in an undefined state.

That'd be a completely reasonable resolution 🙂

compareAndSet would require a CAS loop - might be a pessimization for the common case?

Yep, understood. You folks got benchmarks we could check this against? Suspect ours are a bit high-level, might get lost in noise.

No urgency on our part, we've put more guards in 🙂

[lidavidm]

I'm not too sure if we have benchmarks - coverage is spotty for this repo. There's also the question of the right benchmark (frankly, I would assume an application should not be spamming reference count updates, in which case a CAS loop would presumably not cost anything over an atomic add?).

I would love to use the new FFM APIs instead...

[lidavidm]

@jbonofre not sure if you have opinions or experience from your arrow-java application :)

[jarohen]

I would love to use the new FFM APIs instead...

That's what we're starting to do here 🙂

[lidavidm]

Would resetting the count before throwing be a reasonable solution for your case? That way we only touch the error path

[lidavidm]

I would love to use the new FFM APIs instead...

That's what we're starting to do here 🙂

I would like to hear more if there's anything that can be shared :)

EDIT: there's an existing request: #511

[jarohen]

hey @lidavidm - turns out we had to put this fix in in the end anyway (see linked commits) because it led to problems on our longer running environments.

fwiw the CAS loop didn't seem to add anything to our benchmarks AFAICT 🙂