Skip to content

fix: delay request if process not ready yet #340

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
+74 −3
Draft

fix: delay request if process not ready yet #340

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f270e45
fix: delay request if process not ready yet
quyenvsp Jul 8, 2025
f8d4326
test: request before server initialized
quyenvsp Jul 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion packages/custom_lint/lib/src/v2/custom_lint_analyzer_plugin.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ class CustomLintServer {
BehaviorSubject<SocketCustomLintServerToClientChannel?>();
final _contextRoots = BehaviorSubject<AnalysisSetContextRootsParams>();
final _runner = PendingOperation();
final _delayedRequest = <Request>[];

/// A shorthand for accessing the current list of context roots.
Future<List<ContextRoot>?> get _allContextRoots {
Expand Down Expand Up @@ -186,7 +187,10 @@ class CustomLintServer {
orElse: () async {
return _runner.run(() async {
final clientChannel = await _clientChannel.safeFirst;
if (clientChannel == null) return null;
if (clientChannel == null || !clientChannel.initialed) {
_delayedRequest.add(request);
return null;
}

final response =
await clientChannel.sendAnalyzerPluginRequest(request);
Expand Down Expand Up @@ -291,6 +295,7 @@ class CustomLintServer {
return _closeFuture = Future(() async {
// Cancel pending operations
await _contextRoots.close();
_delayedRequest.clear();

// Flushes logs before stopping server.
await _runner.wait();
Expand Down Expand Up @@ -396,6 +401,14 @@ class CustomLintServer {
await clientChannel.init(
debug: configs.any((e) => e != null && e.debug),
);
_sendDelayedRequest();
}

void _sendDelayedRequest() {
for (final request in _delayedRequest) {
unawaited(_handleRequest(request));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of all of this logic if we're unawaiting the requests anyway?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
_delayedRequest.clear();
}

Future<void> _handleEvent(CustomLintEvent event) => _runner.run(() async {
Expand Down
7 changes: 7 additions & 0 deletions packages/custom_lint/lib/src/v2/server_to_client_channel.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ class SocketCustomLintServerToClientChannel {
final CustomLintWorkspace _workspace;

AnalysisSetContextRootsParams _contextRoots;
bool _initialed = false;

/// Initial state
///
/// Returns `true` if requested `analysis.setContextRoots`
bool get initialed => _initialed;

late final Stream<CustomLintMessage> _messages = _channel.messages
.map((e) => e! as Map<String, Object?>)
Expand Down Expand Up @@ -127,6 +133,7 @@ class SocketCustomLintServerToClientChannel {
sendAnalyzerPluginRequest(_version.toRequest(const Uuid().v4())),
sendAnalyzerPluginRequest(_contextRoots.toRequest(const Uuid().v4())),
]);
_initialed = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of this. It's a bit of a duplicate compared to fields like _processFuture.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All requests naturally await that the process is initialised. That logic is redundant, as the communication channel already handles it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requests are ensured to be sent in the right order. All requests wait for the client to be fully initialized.

}

/// Updates the context roots on the client
Expand Down