Only decrement activeRequestCount on SetTypings responses#18491
Conversation
|
@amcasey, |
| @@ -0,0 +1,1342 @@ | |||
| /// <reference path="../refactorProvider.ts" /> | |||
There was a problem hiding this comment.
Did you mean to include this file?
There was a problem hiding this comment.
I always forget that checkout -f doesn't delete new files. 😢
InvalidateCache responses are triggered by file watchers, rather than by requests.
66bb201 to
f3411d4
Compare
| } | ||
|
|
||
| if (this.logger.hasLevel(LogLevel.verbose)) { | ||
| this.logger.info(`Skipping defunct request for: ${queuedRequest.operationId}`); |
There was a problem hiding this comment.
This still seems like a hazard. You're effectively decrementing by one above to keep track of request/response pairings, yet in this loop you just shift off requests until you find the one you want (with a log "info" message that a request was skipped). Wouldn't this get the activeRequestCount out of sync if it skips them?
There was a problem hiding this comment.
Oh, don't mind me. I see this is scheduling the next one from the queue, so should be good.
There was a problem hiding this comment.
Though one concern/hazard: This while loop previously was always entered to schedule the next request, other than for the 3 event types explicitly handled in the "if" blocks above. Now you only enter this loop to continue scheduling work "if (response.kind === ActionSet)". Shouldn't it continue to always enter this loop except for any events? (Even if only for future proofing?)
There was a problem hiding this comment.
I can change the check to !invalidate if you think that would be more future proof. Otherwise, it's one-in-one-out, so it doesn't make sense to pull anything off the queue if the message doesn't indicate the completion of a request.
There was a problem hiding this comment.
Seems like letting the type system verify exhaustive checks on the string literal types in "TypingInstallerResponse" would be safest to ensure any additions/changes to "response.kind" aren't missed (e.g. https://stackoverflow.com/questions/39419170/how-do-i-check-that-a-switch-block-is-exhaustive-in-typescript ). Possible here?
|
Oops, forgot we were going to harden against new message types. Will post a new PR shortly. |
InvalidateCache responses are triggered by file watchers, rather than by requests.
When I port this to 2.5, I plan to remove the
Debug.failcall.