Resolve "WebIDE displays blank file incorrectly" (!33391) · Merge requests · GitLab.org / GitLab

Tomas Vik requested to merge 214824-fix-web-ide-open-file-race-condition into master May 29, 2020

What does this MR do?

This MR is preventing the Web IDE editor from being initialized before raw file data is fetched. Otherwise, the user would be presented with an empty editor instead.

The MR is done in four commits, first is only refactoring, second implements the fix, third is improving how we toggle the file.loading flag and fourth is adding a functional test against regression. It's possibly easier to review them separately.

TL;DR You can only read Opening two files quickly one after the other for the problem description and Chosen solution for solution explanation.

I'm sorry for the information overload below, it's been a week worth of investigation.

Problem in detail

The problem is that the editor initialization can be triggered when the last opened file is still being fetched. This results in creating an empty model and when the file.raw data comes in, we don't update the model which is cached.

This was happening in two scenarios:

Opening two files quickly one after the other

Set file A as active (i.e. open it by clicking at the tree item)
The editor starts fetching file A
Set file B as active
The editor starts fetching file B
File A is fetched and we initialize the Editor
- the problem is that editor.file property is already set to file B, so we initialize editor for file B before we have the B content
File B is fetched, but we don't update the editor model with the content

Switching `viewMode` whilst the file is still loading

Very similar to the previous scenario, but this time the Editor initialization is triggered by the watcher for viewer property

Possible solutions

Hacks
- use the opposite condition from fetch data
  - When call fetchData, we cancel immediately if the following condition is true file.raw || (file.tempFile && !file.prevPath && !fileDeletedAndReadded)
  - This condition signals that fetching is not necessary
  - We could invert this condition and use it to cancel the editor setup (i.e. if fetching is necessary, don't set up the editor)
- update the model after data comes in
  - We could incorrectly initialize the empty model but listen of changes in file.raw and update the model with incoming data
  - This could lead to problems like the user starting to make some changes to the incorrectly empty file and those changes being overridden
Good enough
- See chosen solution
Best
- Move the file content fetching (fetchData) from repo_editor.vue to our actions.
- That would remove the strange discrepancy when dispatching getData() action changes the state as a side-effect and repo_editor.setup relies on that change rather implicitly

Chosen solution

The "good enough" solution is IMO to make sure we don't set up the editor when the file is still loading. The fix contains two changes:

aborting editor setup if the file is still loading
making sure that when getRawFileData action is in progress, the file has file.loading: true property

Drawbacks of the solution

The current implemententaion first fetches file data and then raw file data:

loading:true
call getFileData()
loading: false
loading: true
call getRawFileData()
loading:false

That means there is a very unlikely scenario of the editor being opened in between these two fetches and the original bug still manifesting itself. Ideally, we would do the fetch atomically, having the loading: true attribute continually throughout both fetches. That's not easy to implement because the getFileData() action is used separately as well.

Another drawback is that we implicitly rely on the fact that when the file finishes loading, we are going to initialize the editor.

this.fetchFileData()
  .then(() => {
    this.createEditorInstance();
  })

In other words, if the editor.file.loading is true, but createEditorInstance() is called, we just assume that we can discard this initialization because createEditorInstance() will be called again when the file is done loading.