fix: delay request if process not ready yet #340
Conversation
|
@quyenvsp is attempting to deploy a commit to the Invertase Team on Vercel. A member of the Team first needs to authorize it. |
|
To view this pull requests documentation preview, visit the following URL: docs.page/invertase/dart_custom_lint~340 Documentation is deployed and generated using docs.page. |
It will get a response though. We can't just ignore those requests. This doesn't seem right. I lack knowledge about the issue though. What exactly are we trying to fix here? |
|
Thank you for reply,
it will wait for the process to start, but the problem occurs that the process is terminated before it can responds. Client Request -> Process start ... -> Process terminated before response (by run flutter pub get/upgrade, start Debug,...) -> New process will start again but old request is bypassed (but still waiting for a response) -> It block
I tested it a lot and it doesn't affect anything. Just make sure But if you feel it is not right, then I will try to test and find a more optimal solution instead of dropping the request. |
Then rather than silencing those requests, we should abort them when the process is terminated. |
quyenvsp
force-pushed
the
fix/request_wait_forever
branch
from
6e0b235 to
f270e45
Compare
quyenvsp
changed the title
| @@ -127,6 +133,7 @@ class SocketCustomLintServerToClientChannel { | |||
| sendAnalyzerPluginRequest(_version.toRequest(const Uuid().v4())), | |||
| sendAnalyzerPluginRequest(_contextRoots.toRequest(const Uuid().v4())), | |||
| ]); | |||
| _initialed = true; | |||
There was a problem hiding this comment.
Not a fan of this. It's a bit of a duplicate compared to fields like _processFuture.
There was a problem hiding this comment.
_procesFuture only tells whether the process has run or not (even if it has errors), we need a flag to know that the process has run without errors and the analysis.setContextRoots request has been sent.
The logic I'm going for is that all other requests should be sent after analysis.setContextRoots, to ensure those requests have context, so we need to delay them and send later.
anyway it's a typo, Windows can't write test, I'm installing Linux and will ping you when fixed
There was a problem hiding this comment.
All requests naturally await that the process is initialised. That logic is redundant, as the communication channel already handles it.
There was a problem hiding this comment.
it was sent but in the wrong order, I am sorting them (instead of drop them) in the correct order because any AnalyzerPluginRequest must be sent after analysis.setContextRoots.
before (wrong order):
setSubscriptions1
setSubscriptions2
updateContent1
updateContent2
analysis.setContextRoots
updateContent3
updateContent4
after (setContextRoots must be first):
analysis.setContextRoots
setSubscriptions1
setSubscriptions2
updateContent1
updateContent2
updateContent3
updateContent4
|
|
||
| void _sendDelayedRequest() { | ||
| for (final request in _delayedRequest) { | ||
| unawaited(_handleRequest(request)); |
There was a problem hiding this comment.
What's the point of all of this logic if we're unawaiting the requests anyway?
There was a problem hiding this comment.
This logic is same _requestSubscription, we just need replay request after delay, so no need to await or handle response. I also added test for this MR
Fixes rrousselGit/riverpod#3582 dart-lang/sdk#55281
When
custom_lintsend request before client process started it will never got response and stuck at analysis.setContextRoots, and block all later request.e.g:
analysis.setSubscriptions,analysis.setPriorityFiles,analysis.updateContentBEFORE client process started