`<iterator>`: `istreambuf_iterator` violates [res.on.data.races]

[CaseyCarter]

The existence of mutable members:

STL/stl/inc/iterator

Lines 480 to 482 in a1bc126

	mutable streambuf_type* _Strbuf; // the wrapped stream buffer
	mutable bool _Got; // true if _Val is valid
	mutable _Elem _Val; // next element to deliver

modified by const member functions without any kind of mutual exclusion:

STL/stl/inc/iterator

Lines 468 to 477 in a1bc126

	_Elem _Peek() const { // peek at next input element
	int_type _Meta;
	if (!_Strbuf || traits_type::eq_int_type(traits_type::eof(), _Meta = _Strbuf->sgetc())) {
	_Strbuf = nullptr;
	} else {
	_Val = traits_type::to_char_type(_Meta);
	}
	_Got = true;
	return _Val;

is a veritable recipe for data races. We've traditionally considered fixing this to require ABI breakage, but I wouldn't be surprised if there were a way to replace some of the data members with atomics in such a way that we could provide a conforming implementation to new code that doesn't break down completely when linked to old code.

vNext note: Resolving this issue will require breaking binary compatibility. We won't be able to accept pull requests for this issue until the vNext branch is available. See #169 for more information.

This was VSO-454475 / AB#454475.

[frederick-vs-ja]

This was mentioned in WG21-P3138.

According to [istreambuf.iterator.ops]/1, it seems that synchronization or atomicity isn't intended. I guess this case can fall into "unless otherwise specified" in [res.on.data.races]/1, but an LWG issue seems desired to me.

Edit: But it seems that equal should still be data-race-free.

[CaseyCarter]

It's unfortunate that SG9 polled against uniformly relaxing [res.on.data.races] for the * operation of all input-only iterators, having the relaxation for only one or two iterator types seems like a footgun compared to making it a consistent property for a large class of types.

In any case, I think I'd prefer relaxing the requirement that * be const (while still requiring it to be semantically non-modifying so *i is equality-preserving) for input-only iterators instead and leaving [res.on.data.races] alone. @timsong-cpp did you consider that alternative when writing WG21-P3138?

[timsong-cpp]

That was an inconclusive SG1 poll. res.on.data.races only applies to standard-provided iterator types anyway, and there’s a pretty small number of these. I might bring it back at some point, but not having it doesn’t cause me a lot of heartburn.

Barry and I did consider the relaxation. That will require extensive changes all over the place (CPOs, concepts, all the iterator/range adaptors…) and we don’t consider that a worthwhile tradeoff.

[CaseyCarter]

That was an inconclusive SG1 poll.

Yes, not really "polled against" so much as "didn't poll in favor of".

Barry and I did consider the relaxation. That will require extensive changes all over the place (CPOs, concepts, all the iterator/range adaptors…) and we don’t consider that a worthwhile tradeoff.

What, you didn't want to invest weeks of your time and WG21's to fix my design mistake?

[timsong-cpp]

Barry and I did consider the relaxation. That will require extensive changes all over the place (CPOs, concepts, all the iterator/range adaptors…) and we don’t consider that a worthwhile tradeoff.

What, you didn't want to invest weeks of your time and WG21's to fix my design mistake?

It would need to weaken everything starting from indirectly_readable, possibly reopening the discussion about the readability of optional, make the resulting iterator unusable with any existing user-written adaptors (until those are updated), all for a very specific case (an input iterator being shared across multiple threads) that seems to us rather unlikely to arise in practice. If we have to choose between that and synchronization, we'd rather add synchronization.

[CaseyCarter]

What, you didn't want to invest weeks of your time and WG21's to fix my design mistake?

It would need to weaken everything starting from indirectly_readable, possibly reopening the discussion about the readability of optional, make the resulting iterator unusable with any existing user-written adaptors (until those are updated), all for a very specific case (an input iterator being shared across multiple threads)

Or an output iterator, the same arguments apply. I should have noticed this when I was arguing to remove copying from single-pass iterators. In any case, thanks for the proposal that will finally enable us to standardize views::generate!

[CaseyCarter]

Clarifying the discussion of WG21-P3138: this proposal will make an exception to [res.on.data.races]/3 for a particular standard library input iterator's member operator*() const. That operator will be permitted to modify the iterator in a way that would incur data races if called concurrently with other member functions. In other words, this const member function will be effectively non-const as far as thread safety is concerned. This is the opposite of the case of vector::begin(), for example, which is guaranteed to be non-modifying and safe to call concurrently with other non-modifying members of vector despite not being a const member itself.

In theory, we could suggest to LWG that a similar exception be made for istreambuf_iterator::operator*() const. This would only partially address the issue, since == for istreambuf_iterator also has data races in our implementation.

[cpplearner]

Maybe istreambuf_iterator is allowed to result in a data race: istreambuf_iterator accesses a basic_streambuf object, and [iostreams.threadsafety] says that concurrent access to a basic_streambuf object may result in a data race.