fix: unable to remove completed searches#911
Conversation
| } | ||
|
|
||
| if (search.state.includes('Completed')) { | ||
| if (search.state.includes('Completed') |
There was a problem hiding this comment.
the state flags end up being serialized as a comma-separated list, so this value will be something like Completed, Cancelled. the state is guaranteed to contain the Completed flag when it's done, so we shouldn't need to include the different sub-states here
|
|
||
| [NotMapped] | ||
| public bool IsComplete => State.HasFlag(SearchStates.Completed); | ||
| public bool IsComplete => |
There was a problem hiding this comment.
same as below; searches (and transfers 🙃) are guaranteed to get the Completed flag regardless of how it ends, so checking for Completed is enough
| Task<Soulseek.Search> soulseekSearchTask; | ||
| try | ||
| { | ||
| soulseekSearchTask = Client.SearchAsync( |
There was a problem hiding this comment.
this is a subtle bug that i should have been more aware of. as you pointed out in discord, the SearchAsync method is actually not async under the hood, it only returns a Task. most of the public methods in Soulseek.NET are like this to disambiguate the behavior of guard clauses, which are designed to throw immediately at the call site, instead of when the resulting task is awaited.
we'll want to keep an eye out for similar cases. nice catch!
| /// </summary> | ||
| /// <param name="search">The search to force cancel.</param> | ||
| /// <returns>The operation context.</returns> | ||
| public async Task ForceCancel(Search search) |
There was a problem hiding this comment.
after reviewing the existing search code i agree that this is necessary right now, but long term i'll create an issue to refactor searches so it isn't possible for them to fail and remain "stuck"
81b962a to
2c7affb
Compare
This PR aims to fix issue #898.
Changes:
A search is now considered completed if search state is either one of these:
Added a try-catch around Client.SearchAsync to disallow bad requests to be eternally at search state 'Requested'.
We may need to find a way to clean-off requests that are already stuck at state 'Requested'.Maybe we could periodically put searches that are at state 'Requested' with a 'DateTime.Now' - 'StartedAt' exceeding a configured timeout value?
Edit: I added a 'ForceCancel' on Searches that allows to cancel searches that are at state 'Requested' with a 'DateTime.Now' - 'StartedAt' exceeding the configured Soulseek.Connection.Timeout.Inactivity option when TryCancel returns false.
Meaning that users that have stucks requests will be able to cancel them then remove them manually.