[C++] Possible data race in accessing scalars

[mpimenov]

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

I use an arrow::compute::literal in my parquet reading pipeline and get ThreadSanitizer reports about this code:

arrow/cpp/src/arrow/array/data.cc

Lines 288 to 290 in c23a097

	auto* offsets = reinterpret_cast<offset_type*>(scratch_space);
	offsets[0] = 0;
	offsets[1] = static_cast<offset_type>(value_size);

If I am reading it right, this results in issuing write instructions during what would seem to be read-only accesses, and this indeed looks racy.

Component(s)

C++, Parquet

[pitrou]

Can you show your stack trace?

[mpimenov]

Arrow is built from commit 3c655df
tsan-40069.txt

[pitrou]

Thank you. It looks like calling ArraySpan::FillFromScalar from several threads is legitimate when the same scalar is used as input for different computations. Given that this is probably always writing the same value, it would probably be enough to use relaxed atomic stores.

@bkietz

[zanmato1984]

Thank you. It looks like calling ArraySpan::FillFromScalar from several threads is legitimate when the same scalar is used as input for different computations. Given that this is probably always writing the same value, it would probably be enough to use relaxed atomic stores.

@bkietz

@pitrou Just a thought, seems like the information to fill into the scalar's scratch space is totally self-contained, should we do the filling by a (virtual) member function of the scalar class, e.g. virtual BufferSpan Scalar::FillScrach(...)? And we can probably use an acquire-release atomic flag to mark if the scratch has already been filled. This might be more semantically clear imho.

(But this may bring ABI issues, which I'm not sure how is dealt with in arrow.)

[pitrou]

@bkietz Would you be able to take a look at this?

[pitrou]

@zanmato1984 I'm not sure about the answer to the rest of your question, but for the record Arrow C++ doesn't promise any ABI stability. We do try to maintain API compatibility (with the occasional deprecation or signature change), but people are expected to recompile when switching from one version of Arrow C++ to another.

[zanmato1984]

@zanmato1984 I'm not sure about the answer to the rest of your question, but for the record Arrow C++ doesn't promise any ABI stability. We do try to maintain API compatibility (with the occasional deprecation or signature change), but people are expected to recompile when switching from one version of Arrow C++ to another.

Thank you for the info.

After some attempts to draft a fix, I found something worth discussion.

First I was able to reproduce the TSAN error using a simple UT. Then I tried to do a plane fix proposed by @pitrou , that is using relaxed atomic stores. Given that I'm not aware of how to apply atomic operations on plane memory in C++, I'll have to change the definition of the scratch space into several atomic variables. However, the interpretation of the scratch space is type-dependent, i.e., some types treat it as two 32-byte space and others treat it as two 64-byte space. This makes "defining the scratch space as a fixed number of atomic variables in the class" not feasible.

Now I'm drafting the alternative way, which is using extra atomic flags + spinning (keeping scratch space itself unchanged - as plane memory), which is basically what I talked about in my previous comment. It'll come later.

[pitrou]

However, the interpretation of the scratch space is type-dependent, i.e., some types treat it as two 32-byte space and others treat it as two 64-byte space.

But all threads accessing the scratch space should interpret it as the same type, right?

extra atomic flags + spinning (keeping scratch space itself unchanged - as plane memory)

Please let's avoid home-grown synchronization like this. Either we can use atomic accesses without spinning, or we should use a lock.

[zanmato1984]

However, the interpretation of the scratch space is type-dependent, i.e., some types treat it as two 32-byte space and others treat it as two 64-byte space.

But all threads accessing the scratch space should interpret it as the same type, right?

Right. The question here is that the scratch space is defined in a generic place, i.e., ArraySpanFillFromScalarScratchSpace (base class of all "scratched" scalars) and we couldn't know how it is going to be interpreted at runtime, despite whether all threads interpret it as the same type or not.

extra atomic flags + spinning (keeping scratch space itself unchanged - as plane memory)

Please let's avoid home-grown synchronization like this. Either we can use atomic accesses without spinning, or we should use a lock.

Yeah, of course. I was thinking about using mutex too.

[pitrou]

Right. The question here is that the scratch space is defined in a generic place, i.e., ArraySpanFillFromScalarScratchSpace (base class of all "scratched" scalars) and we couldn't know how it is going to be interpreted at runtime, despite whether all threads interpret it as the same type or not.

It would be incorrect for different threads to interpret it differently, even with a lock.

[zanmato1984]

Right. The question here is that the scratch space is defined in a generic place, i.e., ArraySpanFillFromScalarScratchSpace (base class of all "scratched" scalars) and we couldn't know how it is going to be interpreted at runtime, despite whether all threads interpret it as the same type or not.

It would be incorrect for different threads to interpret it differently, even with a lock.

You are right about this too.

I think here are two things:

1. Is the scratch being interpreted identically by different threads?
2. If 1 is yes, is it a race?

For 1, if the answer is no, then it should fall into some sort of undefined behavior area. That will be a user code problem that is using scalar the wrong way. It is not essential whether this is a race, or how the synchronization is implemented. And hopefully, and fortunately, I think current compute and Acero components are using scalar the "right way". So we don't really need to worry about it.

For 2, this is what we are really talking about. Aren't we :)