Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix subtle stale state issue in H5WasmProvider #1568

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Feb 8, 2024

We received the following bug report in myHDF5:

I have problem when loading two HDF5 files of different type into the myHDF5 page. If I look at the files one-by-one there is no problem.

Picture (Device Independent Bitmap) 1

When opening the first file then the second file, a bunch of errors are logged to the console that seem to suggest that the entity at the default path is not found due to an incorrect identifier. Important detail: both files have the same default NeXus path.

After some investigation, I identified that the problem comes from a stale state issue in H5WasmProvider: upon receiving new prop values (fileName and buffer), an effect runs in order to create a new API instance, update it in local state, and clean-up the old API instance. However, since this is done asynchronously, the App gets rendered once with the old API instance.

I think the reason why this hadn't caused any issue until now is because it only occurs when switching between two files with the same default path. If the default path of the second file is different, then the old API instance can't find a corresponding entity in the file and stops there silently. On the contrary, if the default path is the same, then the old API instance finds an entity, which means that the viewer then tries to read it... but the internal ID of the entity returned by the old API instance doesn't match the ID of the entity at the same path in the second file, which throws an error.

The difficulty in finding a solution comes from the fact that the API needs to be cleaned up, which typically means useEffect... Unfortunately, the following solutions were not satisfactory:

  • current implementation (useState + useEffect) + requiring the consumer to set a key on H5WasmProvider 👉 this would rely on the consumer to compute the key correctly (fileName is not sufficient, since it's possible to open two files with the same name, and React doesn't accept an object like buffer as key)
  • useMemo (instead of useState) + useEffect with only a clean-up function 👉 this is not compatible with StrictMode as it leads to the clean-up function being called right away on the memoised api instance in development.

In the end, I've landed on a solution that involves useMemo and a synchronous setState call. The App is never rendered with a stale API instance because the synchronous setState call tells React to stop rendering the tree; the H5WasmProvider component function is run to completion (so the clean-up call that follows is called as expected), but it stops there and re-renders right away.

@axelboc axelboc force-pushed the fix-lifecycle branch 4 times, most recently from 504b20e to d33b6c6 Compare February 8, 2024 11:21
When any of the props change, `App` gets rendered once with a stale API instance.
Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a convoluted bug

@axelboc axelboc merged commit 5b44302 into main Feb 8, 2024
8 checks passed
@axelboc axelboc deleted the fix-lifecycle branch February 8, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants