You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2021/01/08 14:20:30 UTC
[GitHub] [netbeans] sdedic opened a new pull request #2650: Hotfix: remove redundant check that causes LSP tests to fail.
sdedic opened a new pull request #2650:
URL: https://github.com/apache/netbeans/pull/2650
Since the patch removes `assert`, it deserves some explanation. The thing to remember up front is: in normal NetBeans IDE or NetBeans application that uses Parsing modules, there's just **a single instance** of `SourceFactory` interface, and that instance has just **a single** cache. By default `DefaultSourceFactory` in Parsing API in `implspi` package (which is NOT public, btw).
The interface was introduced during API/UI split in the past, and one of the driving scenarios were server applications (hey, LSP is another of them !) that could _eventually_ support multi-tenancy. The check was there to ensure that a retruned `Source` buffer really belongs to the calling context.
We do not have multi-tenancy server scenario (any more): It never was in NB IDE and the offspring projects died one way or another, so for NB and LSP server (which is not multi-tenant in any way) this is safe.
Evem from the original perspective, the `assert` is not well placed: the factory itself should check whether the passed context is appropriate, and potentially throw a `IllegalStateException` if it wants to reject the request.
Now why it is bad: each **session** of LSP (or any incoming command stream) is likely to carry some information, e.g. the remote client connected to the NB server instance. There's even a provision in OpenAPI: `Lookups.executeWith()` that establishes a new instance of `Lookup.getDefault()` and `RequestProcessor.post` carries that instance over to offspring tasks.
A similar requirement is *very likely* to be for a single request: that temporary Lookup can gather per-request resources, so the computations invoked during request can share or cache data, including offsprings so the execution environment is *stable*. There's a way how the code can break out to 'global' context, so true daemons without any client (or context) ties can be done cleanly.
In [PR-2641](https://github.com/apache/netbeans/pull/2641), I have introduced such per-request context, to capture `workDoneProgressToken` that may come from virtually any client's `RequestMessage`. A Progress API call can then acquire and connect to such a token so the client may nicely connect the progress to the invoked operation. There will be probably more usages for this 'request-scope' in the future.
But - then `Source` instances cached by `DefaultSourceFactory` that were created during **different** requests had their `context` (Lookup) instances **different**, although they originated from the same user session (it's slightly different in test suite, explained below); so a Source created in some previous LSP request, obtained from the cache, fails the check.
The check is **unnecessarily strict**: it should ensure that the 'scope' of the execution that wants to use the `Source` is the same as the scope that had created it (e.g. the same user/tentant). That means it really wants to ensure that "something in the Lookup's contents" is the same (or equal) to the appropriate 'thing' in the source returned from the Factory. But - the parsing api **does not have any representation** of that 'thing'; and possibly cannot (shouldn't) have.
So my decision is that the check is really a job of the `SourceFactory` implementor since that implementation has access to the scope-private definitions that can identify or differentiate one scope from another.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] sdedic merged pull request #2650: Hotfix: remove redundant check that causes LSP tests to fail.
Posted by GitBox <gi...@apache.org>.
sdedic merged pull request #2650:
URL: https://github.com/apache/netbeans/pull/2650
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] sdedic edited a comment on pull request #2650: Hotfix: remove redundant check that causes LSP tests to fail.
Posted by GitBox <gi...@apache.org>.
sdedic edited a comment on pull request #2650:
URL: https://github.com/apache/netbeans/pull/2650#issuecomment-756782805
Now why the tests are different: the testsuite actually makes **one LSP connection per test case**. In our view of how the `Source` cache should behave for a LSP server, all those clients (e.g. multiple `vscode` instances) would share the cache: at the end of the day, requests to the same `FileObject` will point to the same place on the disk, so the project environment is the same (well, hypothetically two clients with different sets of libraries or JDK requirements may come in, but the LSP server is far from such complexities).
I've even prototyped such global cache for `Source` objects, see https://github.com/sdedic/incubator-netbeans/tree/prototype/lsp-client-session-sources: all the `Source` objects received the true global `Lookup` as their context. But it turned out, that some code in NetBeans still invokes `Source.create()` without any context information, thus passing the effective request-specfic Lookup to the factory (which is OK), and compares *that specific Lookup* to the returned Source's context ... which is a bug.
As explained above, the check should have checked whether the returned `Source`'s contex is is "compatible" (allowed, ..) in the calling context. And Parsing API simply does not have information to do that check - pure identity check is far too strict.
Uff.
So many words for a one-liner fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] sdedic edited a comment on pull request #2650: Hotfix: remove redundant check that causes LSP tests to fail.
Posted by GitBox <gi...@apache.org>.
sdedic edited a comment on pull request #2650:
URL: https://github.com/apache/netbeans/pull/2650#issuecomment-756782805
Now why the tests are different: the testsuite actually makes **one LSP connection per test case**. In our view of how the `Source` cache should behave for a LSP server, all those clients (e.g. multiple `vscode` instances) would share the cache: at the end of the day, requests to the same `FileObject` will point to the same place on the disk, so the project environment is the same (well, hypothetically two clients with different sets of libraries or JDK requirements may come in, but the LSP server is far from such complexities).
I've even prototyped such global cache for `Source` objects, see https://github.com/sdedic/incubator-netbeans/tree/prototype/lsp-client-session-sources: all the `Source` objects received the true global `Lookup` as their context. But it turned out, that some code in NetBeans still invokes `Source.create()` without any context information, thus passing the effective request-specfic Lookup to the factory (which is OK), and compares *that specific Lookup* to the returned Source's context ... which is a bug.
As explained above, the check should have checked whether the returned `Source`'s contex is is "compatible" (allowed, ..) in the calling context. And Parsing API simply does not have information to do that check - pure identity check is far too strict.
Uff.
So many words for a one-liner fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] sdedic commented on pull request #2650: Hotfix: remove redundant check that causes LSP tests to fail.
Posted by GitBox <gi...@apache.org>.
sdedic commented on pull request #2650:
URL: https://github.com/apache/netbeans/pull/2650#issuecomment-756782805
Now why the tests are different: the testsuite actually makes **one LSP connection per test case**. In our view of how the `Source` cache should behave for a LSP server, all those clients (e.g. multiple `vscode` instances) would share the cache: at the end of the day, requests to the same `FileObject` will point to the same place on the disk, so the project environment is the same (well, hypothetically two clients with different sets of libraries or JDK requirements may come in, but the LSP server is far from such complexities).
I've even prototyped such global cache for `Source` objects, see https://github.com/sdedic/incubator-netbeans/tree/prototype/lsp-client-session-sources: all the `Source` objects received the true global `Lookup` as their context. But it turned out, that some code in NetBeans still invokes `Source.create()` without any context information, thus passing the effective request-specfic Lookup to the factory (which is OK), and compared *that specific Lookup* to the returned Source's context ... which is a bug.
As explained above, the check should have checked whether the returned `Source`'s contex is is "compatible" (allowed, ..) in the calling context. An Parsing API simply does not have information to do that check - pure identity check is far too strict.
Uff.
So many words for a one-liner fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] sdedic merged pull request #2650: Hotfix: remove redundant check that causes LSP tests to fail.
Posted by GitBox <gi...@apache.org>.
sdedic merged pull request #2650:
URL: https://github.com/apache/netbeans/pull/2650
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] sdedic commented on pull request #2650: Hotfix: remove redundant check that causes LSP tests to fail.
Posted by GitBox <gi...@apache.org>.
sdedic commented on pull request #2650:
URL: https://github.com/apache/netbeans/pull/2650#issuecomment-756782805
Now why the tests are different: the testsuite actually makes **one LSP connection per test case**. In our view of how the `Source` cache should behave for a LSP server, all those clients (e.g. multiple `vscode` instances) would share the cache: at the end of the day, requests to the same `FileObject` will point to the same place on the disk, so the project environment is the same (well, hypothetically two clients with different sets of libraries or JDK requirements may come in, but the LSP server is far from such complexities).
I've even prototyped such global cache for `Source` objects, see https://github.com/sdedic/incubator-netbeans/tree/prototype/lsp-client-session-sources: all the `Source` objects received the true global `Lookup` as their context. But it turned out, that some code in NetBeans still invokes `Source.create()` without any context information, thus passing the effective request-specfic Lookup to the factory (which is OK), and compared *that specific Lookup* to the returned Source's context ... which is a bug.
As explained above, the check should have checked whether the returned `Source`'s contex is is "compatible" (allowed, ..) in the calling context. An Parsing API simply does not have information to do that check - pure identity check is far too strict.
Uff.
So many words for a one-liner fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists