Fix places awaiting void, for dart 2.

[MichaelRFairhurst]

Looks like cache.invalidate does do some asynchronous stuff
(splitter.close(), but notably not _stale.close() which returns void).
Return the future for that stuff without changing its order of
operations.

Also some place awaiting expect() which appeared to be intended to be
awaiting cancel(), switching those still passes tests.

[nex3]

I'm not really actively maintaining this package anymore.

[MichaelRFairhurst]

no problem @nex3, you've reviewed a ton already today anyway!

Not sure if @matanlurey is a good reviewer? Threw @kevmoo on it to have a big reviewing party.

[kevmoo]

I'd just make the function async and await here – in theory you could be avoiding some unneeded async silly if the splitter != null – but you are also adding some complexity to how this method reads.

Maybe add an await and put the null check in a block – best of both worlds?

[kevmoo]

Also this is a ~breaking change – folks will start getting warnings about awaiting here if they have the lint enabled.

Why is this change needed?

[MichaelRFairhurst]

Well, technically I would say this is a bugfix.

If the invalidate method does asynchronous work, and the future of that work is thrown away, its impossible to await invalidation. There is at least some code (in the tests) awaiting validation, and that silently did not actually await the invalidation.

However, this could be maintained as returning void, and we could add a invalidateAsync method which returns the future of _cachedStreamSplitter?.close() ?

Regarding the order of operations, I just really wanted to land a change that's as small as possible -- so this won't add asychrony in the void case (like before) and it won't change the order of operations vs before. Not necessarily important, and not as clean as awaiting here, but I just didn't want spooky asynchrony changes to stall the CL.

If you aren't spooked by the await I'll do that though! And do you want it to be a new method? The good news is that dart 2 will not let them use the wrong method ("you can't await invalidate() as it returns void")

[MichaelRFairhurst]

ping @kevmoo

[kevmoo]

@matanlurey – thoughts?

[natebosch]

I don't understand how this relates to an await void? Was something in the tests awaiting the result of this function before? Wasn't that effectively await null? Can it still be?

[MichaelRFairhurst]

Yeah, a test was awaiting this method. It could definitely return null, but I would consider the following a bug:

Future f() {
  spawn_async_operation_without_await();
  return null;
}

"function returning future doesn't actually block until work is complete" is the title I'd give that bug!

Alternatively, and I mean this as a strawman but I also honestly prefer it, I could do

Null f() {
  spawn_async_operation_without_await();
  return null;
}

[natebosch]

What I mean is, the old behavior was already translatable to await null; This isn't a case of a Future slipping through a void signature and being an otherwise valid await. This is a case of a Null going through a void signature. I agree on principle with the idea of making this async work possible to await. But as it is this change is not maintaining the same semantics in the test.

[MichaelRFairhurst]

Yeah, I was careful to preserve the ordering semantics just out of conservatism, but didn't do so for this part, which is arguably inconsistent. I can leave a TODO and return null.

[MichaelRFairhurst]

OK done. PTAL

I went with returning Null. I didn't have the heart to commit:

Future<void> fn() {
  // TODO: await this
  async_op();
  // TODO: return a future
  return null;
}

[natebosch]

how about Future<void>?

[MichaelRFairhurst]

Thanks, definitely should do that..!

[natebosch]

I don't understand how this relates to an await void? Was something in the tests awaiting the result of this function before? Wasn't that effectively await null? Can it still be?

[natebosch]

I think we can leave this method as it was (although we can either file an issue or add a TODO).

This can continue to be a void return and we can rewrite await foo.invalidate(); to

foo.invalidate();
await null;

[natebosch]

Ah, missed your commend that you had explicitly avoided this. FWIW I prefer it since it localizes the "fix" to where the await is. It's also slightly less "breaking" to later change from void to Future<void> than it would be to change from Null to Future<void>.

[MichaelRFairhurst]

Part of the problem is that if this code should be awaited, because it does do asynchronous work, then I don't want to migrate all code awaiting this to no longer do so, only to then fix it and have to migrate all other code back.

This "other code" would be third party code, not internal code, though.

[natebosch]

We've decided to keep allowing await void for now right? Maybe we should punt on this and file an issue to do the proper fix of making it return a Future?

[MichaelRFairhurst]

We might still get this in in time. But its not a priority