fix(refresh-endpoint): call refresh while refreshing

[apaniel]

Description

changes:

• Queue unique refreshing requests when a refresh request is being processed

Related Issues

Fixes #20576

[xavivars]

@danabrit , @freiksenet , is there anything else needed in this PR to get reviewed?

[freiksenet]

Hi!

Thanks a lot for you PR!

Core team is currently not working because of Gatsby Company Gathering. We'll make sure to review this on Monday next week.

[pieh]

In my case, it is useful for doing health-checks when having the development server in a container as a preview server. Otherwise health-checks can fail because development server cannot respond while refreshing.

@apaniel90vp Can you expand on this? Does your develop server not serving requests when data is being refreshed? This should only really happen if there is massive chunk of sync code being executed as part of refresh (not allowing express app to process and serve requests), but most refreshes are hugely async so that should give room for server to respond to requests

[apaniel]

@pieh

Can you expand on this? Does your develop server not serving requests when data is being refreshed?

no, it doesn't, it hangs until refresh finishes. If multiple requests happen in a row, the development server will take a few minutes to respond, leading us to a failed health-check.

This should only really happen if there is massive chunk of sync code being executed as part of refresh (not allowing express app to process and serve requests), but most refreshes are hugely async so that should give room for server to respond to requests

We are using contentful as source of our data, and we have a huge amount of data there

[pieh]

Ok, so from reading this PR I do think this PR should be split into 2:

• 1 adding queue (and no ui changes, so just queueing part)
• follow up adding optional screen ( I'm really torn about this one, I would much prefer fix core issue about server being unresponsive than put band aid like "we are refreshing" screen). The thing about this screen is that webhook data refreshes are not the only thing that can keep gatsby develop busy. There are some plugins that don't use those for data updates, so data updates from those category of plugins wouldn't be handled by code like that.

Some of my initial thoughts:

You are implementing queue mostly in develop-proxy - in some ways this is nice because you can immediately create response even if gatsby itself is busy/unresponsive - but not sure if that would be desired tbh. I do think that queue should be implemented much closer to code that executes sourceNodes (etc), just not sure if requests can be stalled long enough to not cause other problems elsewhere

[apaniel]

I can split it in two, no problem.

I created the queue in the proxy for immediate response, as I said in earlier comment, we have contentful as source with quite some data and our refresh gets totally busy, so creating that queue in other place didn't work for me.

I saw this refreshing screen in place for config and node changes and just reused it, as you say that could not be the desired experience, that is why I added it behind an env variable.

[apaniel]

@pieh I updated the MR to keep only the queue part, makes much more sense to split it :)

About the optional restarting screen, I still believe it would be helpful, let me explain our case and you can give me your thoughts.

For us having a preview container is more oriented to content authors (non developers), so showing this restarting screen is also useful, because they can know that after having the refreshing screen on sight they will have the page with updated data.

Keeping it behind an env variable for opt in is because we know that could not be the desired behavior for all the use cases, but when gatsby is working as a develop server on a preview container.

Is there any other way to make develop server deployed in a container to reload rather than the /__refresh endpoint? Would there be any other way to let our non developers know that data refresh has finished?

I can totally help on fixing the underlying issue or to expand the optional refreshing screen to other cases if you like, but I think that it is an enhancement on the user experience, specially from a non-developer gatsby-user perspective.

[xavivars]

Hey @pieh,

Thanks for reviewing this!

The problem moving the queue management closer to where the nodes are creating are, as @apaniel90vp says, the whole server gets stuck. The PR is not only to prevent unnecessary refreshes, but also to have a responsive server that will return something (even if it's a temporary unavailable message) instead of just hanging the connection and, eventually, timing out.

Your point about the page (the piece of the initial PR that got removed in the second iteration) it's also very good. This page may not cover all times where Gatsby develop is busy, but it's definitely an (optional, behind a flag) improvement for some cases, like when using gatsby-source-contentful, and can be a good first step towards a wider and more exhaustive management of busy states.

What do you think of the current status of the PR?

[apaniel]

I opened a MR to the same branch, in case someone is interested in the refresh screen that we mention in the comments of this MR:
apaniel#2

[xavivars]

hey @pieh, @freiksenet,

With the latest changes @apaniel90vp had to do a small change in the PR, but the build is failing in Gatsby Cloud and we don't know anymore what the problem is....

https://github.com/gatsbyjs/gatsby/pull/24887/checks?check_run_id=798498038

[apaniel]

A piece that I'm missing is how to get the CYPRESS_PROJECT_ID and CYPRESS_RECORD_KEY for a new cypress e2e test, so they are recorded in the cypress dashboard

[xavivars]

Hi!

Thanks a lot for you PR!

Core team is currently not working because of Gatsby Company Gathering. We'll make sure to review this on Monday next week.

@freiksenet , any updates?

[wardpeet]

Hey, @freiksenet is currently out for the week. He'll get back to this PR when he's back.

What's the reason for creating a new e2e test and not just adding it to e2e_tests_development_runtime?

[xavivars]

Thanks for the information, @wardpeet! We didn't know if there was any step missing for this to get reviewed 😉

Regarding the reason of creating a new set of e2e tests, @apaniel90vp will need to chime in (he's also out today 😅).

[pieh]

I was thinking more about this- primarily having queue in parent process because of unresponsiveness of actual express app during refresh. I think I would prefer to change Contenful plugin so it doesn't block event loop for long periods of time and instead use something like this -

gatsby/packages/gatsby/src/schema/index.js

Lines 72 to 73 in b73cc5d

	// Give event-loop a break
	setTimeout(processNextType, 0)

as it does work to let other parts of gatsby react to external events. This would mean that gatsby is fully still usable (slower, sure, but usable) during refresh as opposed to this approach that allow just refresh requests to come through. To be fair - it might make sense to do both as blocking event loop is likely not limited to Contentful plugin. Adding ability to flush event loop periodically would also mean there would be no need for that optional "we are working" screen that you originally had in this PR beucase gatsby would still be able to handle response (might be less snappy, but shouldn't timeout completely).

A piece that I'm missing is how to get the CYPRESS_PROJECT_ID and CYPRESS_RECORD_KEY for a new cypress e2e test, so they are recorded in the cypress dashboard

That would need one of the gatsby cypress org admin to set up, but they are not needed per se to land e2e test. But I do think we could integrate this in already existing e2e dev runtime and not create new one - those e2e tests are "expensive" to bootstrap - we already do have some refresh webhook tests there so adding more cases would fit well there

[apaniel]

Hey, sorry for not answering earlier, I've been out until today with limited connection.
To your comments:

What's the reason for creating a new e2e test and not just adding it to e2e_tests_development_runtime?

But I do think we could integrate this in already existing e2e dev runtime and not create new one - those e2e tests are "expensive" to bootstrap - we already do have some refresh webhook tests there so adding more cases would fit well there

The original PR (second part here: apaniel#2) had a new env variable ENABLE_GATSBY_RESTARTING_SCREEN_ON_REFRESH so I needed to spin up a new development process to be able to have this variable.
When I did the split of the MR in two I did't move it to the existing e2e dev runtime because there is no way to know whether the refresh has finished or not, as a way of not affecting other tests I left this new cypress e2e tests.
Just added a new commit moving the new refresh endpoint tests to the existing e2e dev runtime.

To be fair - it might make sense to do both as blocking event loop is likely not limited to Contentful plugin.

I agree, I gave you the contentful plugin example, but any plugin using any gatsby node API and doing any long running operation would block the event loop.

I think I would prefer to change Contenful plugin so it doesn't block event loop for long periods of time and instead use something like this -

I would leave this task for another MR, as it would involve changes in the Contentful plugin

[apaniel]

@freiksenet @pieh @wardpeet is there anything else I can do on my side?

[xavivars]

Any updates here?

May this PR get obsolete due to the changes in https://github.com/gatsbyjs/gatsby/pull/25479/files?

[ascorbic]

@xavivars Yes, I think it's best to hold off on this until #25479 and the other state machine PRs are in. We can then see how much of this makes sense to keep (and whether it's still needed). The state machine work makes some fundamental changes to how we handle develop, which should make things more reliable in all cases, so maybe this won't be needed.

[ridi-mz]

Hi, any updates in this PR ? Will it be merged ?

[seidtgeist]

Well, looks like #25479 has been merged and there are still problems that could be alleviated by not doing multiple parallel refresh passes.

If I call __refresh more than once at the same time, everything goes irreparably wrong:

• First I had OOM errors, so I updated to the latest version of gatsby that fixed some of this via fix(gatsby): leak less in chokidar listeners #27685
• Now I’m hopefully getting less OOM errors (I don’t know to be honest), so I’m getting a new error:

error Missing onError handler for invocation 'building-schema', error was
'Error: Schema must contain uniquely named types but contains multiple types named "XYZ".'

(XYZ being the configured typeName of the gatsby-source-graphql plugin)

I have a feeling that Gatsby as a project might have reached a complexity ceiling a couple of years ago, where the sheer amount of STUFF happening under the hood now made it impossible to be either fast or correct/reliable in any way. Every senior/principal developer I’m talking to is having similar feelings. What’s the feeling inside, I wonder?

[LekoArts]

Hey! Thanks so much for opening this pull request!

Sadly this PR got stale and didn't have any activity for some time. We're trying to do better with PR reviews! To get a better overview of all actionable PRs we're going through all open PRs and triage them. Since we won't be able to do everything and adding new features always means added maintenance burden, we have to be more picky about what's beneficial for the average user and the project itself longterm.

Having said all this, we dropped the ball on this PR and we're sorry about not responding to it earlier. As said above, we want to do better in the future but here we should have communicated earlier what will happen with the PR.

In the meantime Gatsby 5 and related packages were released. You can check our Framework Version Support Page to see which versions currently receive active support. Please try the mentioned issue on the latest version (using the next tag) and if you still see this problem, open a new bug report. It must include a minimal reproduction.

We absolutely want to have you as a contributor and are sorry for any inconveniences we caused with replying too late to this PR.

Thanks for submitting to Gatsby! 💜