Fix MemoryCachingByteStore._evict after an unsuccessful get.

Fix MemoryCachingByteStore._evict after an unsuccessful get.

R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/08fa512ff2c1c48f820bcc84776f54400ee17418

[Paul Berry, 2016-11-22 23:48:15]

paulberry@google.com changed reviewers:
+ scheglov@google.com

[Paul Berry, 2016-11-22 23:50:30]

This should fix fatal errors from the new analysis driver of the form
"NoSuchMethodError: The getter 'length' was called on null." during
MemoryCachingByteStore._evict.

[Brian Wilkerson, 2016-11-23 00:04:27]

brianwilkerson@google.com changed reviewers:
+ brianwilkerson@google.com

[Brian Wilkerson, 2016-11-23 00:04:27]

DBC: Wouldn't it be better to not allow `null` as a value in the map?

[Paul Berry, 2016-11-23 00:09:09]

On 2016/11/23 00:04:27, Brian Wilkerson wrote:
> DBC: Wouldn't it be better to not allow `null` as a value in the map?

I don't know.  There would be a change of semantics though.  Currently, if you
call get(key) and receive null, and then call get with the same key, you will
receive a cached null.  If we disallow null values in the map, then if you call
get(key) and receive null, the null won't be cached, and calling get with the
same key will cause the key to be looked up in the filesystem again.

But maybe we don't care about that semantic difference, since usually a get(key)
that receives null is going to be followed pretty soon afterwards by a put with
the same key.

Konstantin, what do you think?

[scheglov, 2016-11-23 02:54:32]

On 2016/11/23 00:09:09, Paul Berry wrote:
> On 2016/11/23 00:04:27, Brian Wilkerson wrote:
> > DBC: Wouldn't it be better to not allow `null` as a value in the map?
> 
> I don't know.  There would be a change of semantics though.  Currently, if you
> call get(key) and receive null, and then call get with the same key, you will
> receive a cached null.  If we disallow null values in the map, then if you
call
> get(key) and receive null, the null won't be cached, and calling get with the
> same key will cause the key to be looked up in the filesystem again.
> 
> But maybe we don't care about that semantic difference, since usually a
get(key)
> that receives null is going to be followed pretty soon afterwards by a put
with
> the same key.
> 
> Konstantin, what do you think?

It's better to avoid putting null(s) in the first place.

I did not imply about any special semantics for putting null(s).
It's just a bug.

[Paul Berry, 2016-11-23 13:51:19]

On 2016/11/23 02:54:32, scheglov wrote:
> On 2016/11/23 00:09:09, Paul Berry wrote:
> > On 2016/11/23 00:04:27, Brian Wilkerson wrote:
> > > DBC: Wouldn't it be better to not allow `null` as a value in the map?
> > 
> > I don't know.  There would be a change of semantics though.  Currently, if
you
> > call get(key) and receive null, and then call get with the same key, you
will
> > receive a cached null.  If we disallow null values in the map, then if you
> call
> > get(key) and receive null, the null won't be cached, and calling get with
the
> > same key will cause the key to be looked up in the filesystem again.
> > 
> > But maybe we don't care about that semantic difference, since usually a
> get(key)
> > that receives null is going to be followed pretty soon afterwards by a put
> with
> > the same key.
> > 
> > Konstantin, what do you think?
> 
> It's better to avoid putting null(s) in the first place.
> 
> I did not imply about any special semantics for putting null(s).
> It's just a bug.

Turns out I was wrong about the semantic difference.  MemoryCachingByteStore has
always retried after getting `null`.

I went ahead and made Brian's suggested change anyway.  PTAL.

[Brian Wilkerson, 2016-11-23 16:11:40]

lgtm

[scheglov, 2016-11-23 17:00:55]

LGTM

[Paul Berry, 2016-11-23 17:47:48]

Description was changed from

==========
Fix MemoryCachingByteStore._evict after an unsuccessful get.
==========

to

==========
Fix MemoryCachingByteStore._evict after an unsuccessful get.

R=brianwilkerson@google.com, scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/08fa512ff2c1c48f820bcc84776f54400ee17418
==========

[Paul Berry, 2016-11-23 17:47:49]

Committed patchset #2 (id:20001) manually as
08fa512ff2c1c48f820bcc84776f54400ee17418 (presubmit successful).