-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Fix RPC proxy handler notifications and requests order #13810
Fix RPC proxy handler notifications and requests order #13810
Conversation
@rschnekenbu The way I usually do this is via events. See for example: theia/packages/plugin-ext/src/plugin/notebook/notebooks.ts Lines 315 to 333 in c37254f
It doesn't require a timeout (aside from the one that rejects the promise) and should be usable in here as well. You probably just need to introduce a new event emitter that can be used here. |
@msujew thanks for the input, but I think the problem is much more serious: in
Handling of requests is async, while sending notification is synchronous. This can lead to notifications being handled before a request that that has been sent before the notification. Hence we get a notification for a test controller which has not been created yet via a request. |
@tsmaeder Oh I see. Indeed, that is more severe... |
- send notification with a Promise to respect order between requests and notifications - test run profile constructor updates observable properties that send a notification to the main test run profile, without main being told to create this profile. This leads to a profile not found issue when activating the test extension. Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
892ec4a
to
9f8c29b
Compare
It seems that we can have a quick fix for this one (sending notifications also on Promise.then()...), but this may lead to the same issues we had in the test area. Things were working but not always correct. We may have to check if all extension APIs still work with this change. |
With fixes on proxy handler and some other order issues on notification for Test Controller, the sample extension should be running fine. |
Draft PR to discuss TestController initialization issues when dealing with extension
How to test:
On theia master, the extension does not activate nicely. Error messages are displayed in the console, where I get some
No test controller with id mathTestController found
. Looking at the code, the extension registers at the same time the test controller, the test profile and some other things during activation. We get some race condition here between initialization of the TestController on main and its usage when the profile is registered. When profile is registered, testing main uses the withController, where it is not yet available. So the error messages.I pushed a dirty fix, but I would like to discuss with you the best way to handle this issue. Timeout is probably not the best solution, or trying x times is not the best. What would you think as a good implementation here? Knowing of course that we cannot change the way the extension does its activation.
The fix on TestItem is also a test for some tree flickering I get on the Test Items => the extension keeps sending some updates on test items, and as far as I understand the framework, the notifyPropertyChange implementation does not take into account the case where the value is similar to the previous one, causing many tree refreshes. But this part can be ignored for now ;)