RFR(m) 2: 8072722: add stream support to Scanner

[Xueming Shen]

On 09/10/2015 01:22 PM, Stuart Marks wrote:
>
>
> On 9/9/15 3:51 PM, Xueming Shen wrote:
>> One more comment regarding the CME check.
>>
>> 2829                     if (expectedCount != modCount) {
>> 2830                         throw new ConcurrentModificationException();
>> 2831                     }
>>
>> While it definitely serves the purpose of "fail-fast", but given it's a
>> ordered/sequential
>> stream, this condition is always checked in the immediate next round of
>> tryAdvance().
>> Just doubt it really benefits anyone. It's true that it almost costs nothing
>> though.
>
> The stream is initially sequential, but it can be changed to parallel by the application. If this occurs, this spliterator would be wrapped by code that gathers up elements into batches for parallel processing, making the control flow more complicated.
>
> 2826                 // assert expectedCount == modCount
> 2827                 if (findPatternInBuffer(pattern, 0)) { // doesn't increment modCount
> 2828                     cons.accept(matcher.toMatchResult());
> 2829                     if (expectedCount != modCount) {
> 2830                         throw new ConcurrentModificationException();
> 2831                     }
>
> I guess the question is, is it worth having two checks in each call to tryAdvance()? The first check has to be there in order to initialize modCount the first time (see also below), and since you have modCount, you might as well check it if it's nonnegative.
>
> The check after the call to the Consumer might seem unnecessary, but note that there is no guarantee that tryAdvance() will be called again if the stream decides to terminate early. (Consider short-circuiting behavior of findFirst() and friends.) It's easy to see that this "extra" check is a guard against the Consumer modifying the scanner's state, which I think is fairly important.
>
>> On 09/09/2015 03:35 PM, Xueming Shen wrote:
>>> Hi Stuart,
>>>
>>> Can't modCount be increased to negative in extreme case?
>>>
>>> 2705             if (expectedCount>= 0&&  expectedCount != modCount) {
>>> 2706                 throw new ConcurrentModificationException();
>>> 2707             }
>
> (This is in TokenSpliterator.)
>
> Yes, as tokens are processed, modCount will be incremented continually and overflow will eventually occur, causing modCount to wrap around to negative. The expectedCount value is continually reinitialized from modCount, so when this happens, expectedCount will go negative as well. This will disable the CME check at the top of tryAdvance(). If hasNext() is true, though, expectedCount is reinitialized from modCount and it's checked unconditionally after the Consumer call, so that CME check won't be affected. Similar things can occur if modCount has wrapped around to negative before the stream processing starts.
>
> Bottom line is that when modCount is negative, it will bypass half the CME checks in TokenSpliterator. But some checking will still be done.
>
> The situation with FindSpliterator is a bit different. The modCount value isn't incremented while advancing the spliterator, but there is still the possibility that modCount could be negative by the time stream processing starts. If this occurs, each call to tryAdvance() will be treated as the initial case and expectedCount will be reinitialized from modCount. This means that, as before, some CME checking could be bypassed. The check after the Consumer call will still work properly though.
>
> Looks like the apparently redundant CME check after the Consumer call is helping out after all. :-)
>
> It would be nice if this were cleaner, but it still meets the criterion of "best effort". It doesn't appear to fail spuriously, either. I suppose it would halve the number of error cases if expectedCount were compared against its sentinel value of -1 instead of <= 0, but I haven't thought this through. Is it worth it? As things stand, CME checks will be missed only happen after a billion calls on the scanner, and this is only significant if the application starts modifying the scanner illegally at that point.

I think it might be a "nice to have" for a "fail-fast" effort after the the consumer
consumed/accepted the result (the second check), but isn't it a bug for the consumer
to accept any result if there is CME condition occurred already?

>
>>> It'd be better to initialize expectedCount to modCount in constrocutor?
>
> That's how I had it initially, but at Paul Sandoz' suggestion I delayed the initialization to the first call to tryAdvance(). This allows the Scanner's state to be modified after stream creation but before stream pipeline execution. This is the way that Paul's stream code in Matcher works. I'm not sure how important this is. Having Scanner be gratuitously different from Matcher seems like it would be irritating though.
>

I noticed the spec says "Scanning starts upon initiation of the terminal stream operation, using the
current state of this scanner..." guess it means the "CME" enforcement starts with the "stream
operation" starts (a kinda of later-initialization). But personally feel it may create a unnecessary
inconsistent situation, depends on whether or not there is state change between the creation of
the Stream object and the starting of the stream operation. But I'm not a stream expert :-)

-Sherman