New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TryRemoveLastFilterIfNewerThanAsync
should not throw
#12778
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -317,14 +317,14 @@ public async Task<FilterModel[]> FetchBatchAsync(uint fromHeight, int batchSize, | |||
{ | |||
if (!IndexStorage.TryRemoveLast(out filter)) | |||
{ | |||
throw new InvalidOperationException("No last filter."); | |||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is important when there are no blocks in the database, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was some history about why we throw the exception but I can't recall why we did it and given the method's name and what it does I would expect the method NOT to throw, so I think it's a good change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I just added a simple test.
I merged this because it cannot change anything in the current behaviour, but it makes the usage consistent with what can happen. |
Try
implies that the function doesn'tthrow
RemoveAllNewerThanAsync
uses it as if it was not throwing (otherwise, how can it returnnull
???)It's the problem here: #12400 (comment)