Add a timeout for workspace/buildTargets and buildTarget/sources requests
#2277
Conversation
ahoppen
requested review from
bnbarham,
hamishknight and
rintaro
as code owners
|
@swift-ci Please test |
…requests This allows us to provide functionality based on fallback settings for unresponsive BSP servers. Fixes swiftlang#2252
ahoppen
force-pushed
the
bsp-timeouts
branch
from
799d89b to
c142b60
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
1 similar comment
|
@swift-ci Please test Windows |
| changedTargets = Set(request.targets) | ||
|
|
||
| // Caches not invalidated: | ||
| // - cachedAdjustedSourceKitOptions: Same as for buildTargetsReceivedResultAfterTimeout. |
There was a problem hiding this comment.
Assuming that the files are different, we probably do need to clear options here? ie. there may now be added/removed files and thus we need to re-request options.
I also wonder if this is true for build targets - that they have changed could imply that new search paths are needed (or old removed). For SwiftPM this isn't relevant today (as it has the one folder that everything is built into), but for Bazel I imagine this is much more relevant?
And lastly, is it now possible that we have multiple target/source requests in flight in different orders?
There was a problem hiding this comment.
Assuming that the files are different, we probably do need to clear options here? ie. there may now be added/removed files and thus we need to re-request options.
I don’t think so. If we got built settings for a source files then that’s expected to be up-to-date until the build server sends us a buildTarget/didChange notification. In practice I don’t know how we would have gotten here since we won’t request the build settings for a file in a target unless we know that it’s part of the target but even if we did, and the build server was able to return build settings for the file while the buildTarget/sources was still in progress, we would expect that information to be correct until we receive a buildTarget/didChange notification. Does this make sense? It could be that I missed something here. Race conditions and cache invalidation are hard.
I also wonder if this is true for build targets - that they have changed could imply that new search paths are needed (or old removed). For SwiftPM this isn't relevant today (as it has the one folder that everything is built into), but for Bazel I imagine this is much more relevant?
Similar argument here. If we get any build settings for a file, we expect that to be correct until the build server notifies us that they have changed. Consider the workspace/buildTargets as a pure getter request that shouldn’t modify the build server’s state in an observable way. Receiving information about the build targets won’t change the build settings of the files within them.
And lastly, is it now possible that we have multiple target/source requests in flight in different orders?
No more than before. All the timeout handling happens within the closures of the Cache.get calls, so if there are multiple calls to the same target set, they will be served by the same Task, just like before this change. We could have concurrent request for two target sets that aren’t overlapping, eg. sources of targets A, B and another request for targets B, C but then we rely on the build server to give us consistent answers for target B in both requests.
There was a problem hiding this comment.
until the build server notifies us that they have changed
If the assumption is that we expect a buildTarget/didChange, then sounds good to me 👍
| changedTargets = Set(request.targets) | ||
|
|
||
| // Caches not invalidated: | ||
| // - cachedAdjustedSourceKitOptions: Same as for buildTargetsReceivedResultAfterTimeout. |
There was a problem hiding this comment.
until the build server notifies us that they have changed
If the assumption is that we expect a buildTarget/didChange, then sounds good to me 👍
This allows us to provide functionality based on fallback settings for unresponsive BSP servers.
Fixes #2252