NetworkTask: Distinguish implicit abort by timeout from explicit abort #10884
Conversation
There was a problem hiding this comment.
It is unclear what you are trying to fix with this PR. For my understanding, I have issued all your findings in the original PR
Part of a positive review culture is to point out issues and suggest improvements along the original PR. I would be happy to get back to this mode.
I treat the initial sentence as an unfounded insult to my work. I ask you to refrain from doing so immediately.
| @@ -205,11 +205,15 @@ void WebTask::slotStart(int timeoutMillis, int delayMillis) { | |||
|
|
|||
| auto* const pNetworkAccessManager = m_networkAccessManagerWeakPtr.data(); | |||
| VERIFY_OR_DEBUG_ASSERT(pNetworkAccessManager) { | |||
| // Internally switch the state to pending as a prerequisite before | |||
| // invoking onNetworkError(). This is only required for technical | |||
| // reasons and the intermediate state will never become visible. | |||
| m_state = State::Pending; | |||
There was a problem hiding this comment.
Setting the state to Pending here is just a hack to pass the debug assertion in onNetworkError(). The correct solution is to change the state to State::Failed after the previous call fails.
| /// timedOut = false: The abort has been triggered explicitly. | ||
| /// timedOut = true: The abort has been triggered implicitly | ||
| /// after a client-side timeout expired. | ||
| virtual void slotAbort(bool timedOut) = 0; |
There was a problem hiding this comment.
We already have a code path to deliver timeouts to the calling code. Introducing here an anonymous boolean to pass a timeout a different way is unnecessary and raises the code complexity unnecessarily. In addition it exposes implementation details to the calling code. It violates also the code symmetry, because in case of all other error cause the error message is created in the NetworkTask and its derived classes all other Errors are passed in a generic way including a translated message and detailed error codes-
There was a problem hiding this comment.
This is bikeshedding. I already wanted to mention that I was too lazy to introduce an enum class before getting feedback on the proposed solution. I assumed that this is obvious.
There was a problem hiding this comment.
We already have a code path to deliver timeouts to the calling code.
You still confuse client-side with server-side timeouts. A client-side timeout is an abort, but with an implicit instead of an explicit trigger. Technically, it is not an error, because the task did what you told it to do (according to the timeout parameter provided when starting it). A server-side timeout is an unexpected outcome and an error.
Aborting a request might subsequently let your business code fail. But that's a different level of abstraction.
There was a problem hiding this comment.
This is bikeshedding.
Seriously @uklotzde ? Please treat me with respect!
There was a problem hiding this comment.
The client side timeout is an network error in the same way as the server side timeout. There is no need to pass one over a different signal than the other. In contrast the slotAbort is a direct responds to an explicit immediately abort request. Using that for a Network was wrong and causes the bug I have fixed.
But anyway this is an implementation detail f the bug fix.
You have not explained why you experience a bug with it.
|
A simple extension to prevent subversion of the state machine.
https://github.com/mixxxdj/mixxx/pull/10875/files#r965366266