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: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] For further information about the NetBeans mailing lists, visit: https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
