Questionable check for launch.json from file events

[bpasero]

See https://github.com/Microsoft/vscode/blob/b66f0ba3c70b08ffc046a9f1a6f56e0260129540/src/vs/workbench/parts/debug/electron-browser/debugService.ts#L1310

I think this check is no longer a good one because you can have launch configurations in the multi-root workspace file as well. Is this check still needed? It probably should know in which context it is (folder?) and then check specifically for that path and nothing else.

[isidorn]

Good catch that is an old heuristic that on debug restart we read the launch configuration from the launch.json only if that launch.json has been changed, otherwise we use what we have in memory.

In order to solve this properly we could:

• For each launch configuration listen on file events and each launch configuration would be aware if it got changed. Might be a bit ugly since the debug service would have to set for each launch config that when a new debug session starts
• Drop this heuristic and always re-read the launch configuration on restart. @weinand do you remember what was the downside of this approach?

[weinand]

The use case that triggered this was that a user edited a launch config and then aways restarted the debug session to pick up those changes.

Since we get the launch configurations via the configuration service, we should not really have to deal with file events ourselves. Isn't there a general mechanism provided by the configuration service that notifies us to update our in-memory model?

[isidorn]

I understand that use case, but it would be covered by always re-reading before launch. My question is the other way around, what use case is there so we do not read it?

You make a good point about the configuration service, and that is a good approach how to properly solve this on our end and not listen to file events.
However the only time a launch is actually read is when a debug session is started, so my first question holds why not just re-read it before a session starts. Though I am sure there was a reason why we did not simply do this, I just can not remember.

[weinand]

Doing unnecessary work is always bad ("Death by a thousand paper cuts").
If the configuration service gets the data via Live Share, rereading it might be expensive.

Only if the configuration service caches the data for us (and knows when to invalidate the cache), then rereading it every time is acceptable.

[isidorn]

@weinand thanks for advice, I checked with Sandeep and the configuration service always caches.
Also we only re-read on debug session start, so I decided to ignore file events and re-read on debug start.

[weinand]

@isidorn I had to revert this change because it results in a bad regression #48333

[isidorn]

@weinand thanks for the analysis and the fix for that issue.
We need to devise a better strategy for when to re-read and when not, since it is a bit flaky now. I can think about this in debt week.

[weinand]

@isidorn my proposal:
Re-read always but remember old state and compare with new and use new only on change.
(This approach would be simple to implement if we had an object (aka DebugSession) that tracks the whole lifetime of a restarting debug session; I suggest to fix this problem only after introducing DebugSession , see #48377).

For the launch config from #48333 this will still result in the problem (but IMO we can live with that rare corner case).