Avoid hasNext=true on the last incremental payload #588
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gnawf
reviewed
Sep 6, 2024
| return DelayedIncrementalPartialResultImpl.newIncrementalExecutionResult() | ||
| .incrementalItems(emptyList()) | ||
| .hasNext(false) | ||
| .build() |
There was a problem hiding this comment.
I think this belongs in NadelIncrementalResultSupport
This class just accumulates data. This is some incremental result logic.
| @@ -145,21 +143,6 @@ class NadelIncrementalResultSupport internal constructor( | |||
| initialCompletionLock.complete(Unit) | |||
| } | |||
|
|
|||
| fun close() { | |||
| coroutineScope.cancel() | |||
| } | |||
There was a problem hiding this comment.
HUH, this needs to be called. I think it's a bug that it's not.
i.e. we should cancel the jobs launched on the defer scope if the incremental result is cancelled e.g. on request closed early.
We don't have to address that in this PR.
I suspect we can edit resultFlow to something like
fun resultFlow(): Flow<DelayedIncrementalPartialResult> {
return resultFlow
.onCompletion {
close()
}
}Where onCompletion is invoked on cancellation.
| @@ -379,7 +379,9 @@ abstract class NadelIntegrationTest( | |||
| // Note: there exists a IncrementalExecutionResult.getIncremental but that is part of the initial result | |||
| assertTrue(result is IncrementalExecutionResult) | |||
|
|
|||
| // Fuck why delayed & incremental?? Shouldn't incremental == delayed? Why is there an optional synchronous incremental?? | |||
There was a problem hiding this comment.
Sorry, should've deleted that
gnawf
approved these changes
Sep 6, 2024
| subject.copy(hasNext = hasNext) | ||
| } | ||
| // We have to return hasNext=false to indicate to clients that there's no more data coming. | ||
| // Note: the spec allows an empty payload which only contains hastNext=false to be returned. |
There was a problem hiding this comment.
Nit: Javadoc to make comment sticky to function
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The computation of
hasNextis somewhat flaky, as we can see in the occasional failure ofHydrationDeferGroupingTest.The failure occurs because sometimes the last deferred payload contains
hasNext=true.This is possible since not all results from an outstanding job will be converted to a deferred payload and sent to the results channel.
So we might have something like this
The last (and only) event that was actually emitted has hasNext=true, so clients will hang waiting for a completion event forever.
Note: this PR doesn't fix the flaky test (yet)