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

[Bug]: Hot Reload Not Re-enabled When Switching from Production to Development Mode #2457

Open
1 task done
abhishekkumams opened this issue Nov 14, 2024 · 5 comments
Open
1 task done
Assignees
Labels
bug Something isn't working triage issues to be triaged
Milestone

Comments

@abhishekkumams
Copy link
Contributor

What happened?

When updating the environment mode from development to production, hot reload is disabled, which is expected behavior. However, when the environment is subsequently changed back from production to development, hot reload is not re-enabled. This behavior is not expected; ideally, if the initial mode was development, hot reload should automatically reactivate when the mode is reverted to development.

Steps to Reproduce:

  1. Start the application in development mode with hot reload active.
  2. Change the environment mode to production.
  3. Change the environment mode back to development.

Expected Behavior:
Hot reload should be re-enabled automatically when the environment is set back to development mode.

Actual Behavior:
Hot reload remains inactive after switching from production to development mode.

Version

1.3.13-rc

What database are you using?

Azure SQL

What hosting model are you using?

Local (including CLI)

Which API approach are you accessing DAB through?

No response

Relevant log output


Code of Conduct

  • I agree to follow this project's Code of Conduct
@abhishekkumams abhishekkumams added bug Something isn't working triage issues to be triaged labels Nov 14, 2024
@abhishekkumams abhishekkumams self-assigned this Nov 14, 2024
@abhishekkumams abhishekkumams added this to the 1.3 milestone Nov 14, 2024
@seantleonard
Copy link
Contributor

While yes we did consider "Production" -> "Development" mode as part of the scope, this change becomes difficult because there are checks in DAB that evaluate "IsHotReloadEnabled()" which considers dev/prod mode and forbids hot reload when in prod mode. You'd need to signal to hot reload functions that the "Mode" is explicitly changing.

If building in support for this, I'd recommend ensuring that the change includes logging that indicates hot reload is occurring because of a dev/prod mode switch.

Also would be beneficial to check dev mode to prod mode hot reload scenario. There are some instances in startup that are conditioned on Dev mode/prod mode only and those may be skipped when switching mode via hot reload as startup has already completed.

@abhishekkumams
Copy link
Contributor Author

abhishekkumams commented Nov 18, 2024

I gave more thoughts on this and believe its best to keep the existing functionality as this forces a conscious decision to re-enable hot reload and reduces risks associated with inadvertent switches or malicious toggles.

This also aligns with the fact that we retain the scenario when starting the engine with production, switching to development doesn't enable hot-reload automatically.

@aaronburtle
Copy link
Contributor

aaronburtle commented Nov 19, 2024

We can close this issue then if we want to keep the current functionality.

If we decide to change back in the future, because the ability to hot-reload would be gated behind starting the engine in dev mode, it should be "safe" to simply use the built-in functions we have for determining of it is OK to hot-reload or not. This is because if the engine is started in production, file watching will never happen and so there is no danger of somehow a hot-reload doing something unsafe to an instance of DAB. We should be clear when we communicate to customers that for production scenarios, DAB should always be started in production, not hot-reloaded into production. For cases where DAB was started in dev mode, but switched to production, the built-in functions should suffice, since for example when we determine if we should override the open API document, this happens downstream of the hot-reload logic:

Image

So regardless of the mode that the runtime is actually in, which can be arbitrary if the user can swap back and forth with hot-reload, we are always going to over-write the open API document, because we are always in a hot-reload possible scenario, and we know that because we only ever file-watch when the engine is started in dev mode.

So all we have to do is remove any kind of check from the hot-reload logic that looks at the mode. We don't care, we can only ever file monitor if we are starting the engine in dev, so the change is very easy, we just remove the part that checks mode before a hot-reload.

Image

Would love to hear your thoughts, @JerryNixon

@aaronburtle
Copy link
Contributor

aaronburtle commented Nov 20, 2024

Some updates after we had a discussion in a sync up. The current functionality really doesn't make sense. If you start DAB in development mode, then you switch to production mode, you risk having things that are setup for dev mode infiltrating your production scenario.

So, if we want to prevent that from happening, the solution is not to prevent hot-reload out of production mode, it is to prevent the hot-reloading of mode altogether.

This means the two options that make the most sense would be:

  1. The originally intended logic, where if you start dab in dev mode we consider this a development scenario and allow you to freely hot-reload the mode so that you can quickly test how changes you are making will impact things in production mode. With this option the infiltration of dev mode specific properties of dab into the production mode makes sense because you are in a development scenario simply testing how things will work in production mode.
  2. Do not allow for hot-reloading the mode at all, and instead close down the DAB engine if any hot-reload of the mode from development to production was attempted. This prevents any possibility from having features intended for development scenarios to infiltrate into production mode, and if a developer wants to test how the changes theyve been making while in dev mode work in production mode they would have to restart the engine in production mode.

@abhishekkumams
Copy link
Contributor Author

Some updates after we had a discussion in a sync up. The current functionality really doesn't make sense. If you start DAB in development mode, then you switch to production mode, you risk having things that are setup for dev mode infiltrating your production scenario.

So, if we want to prevent that from happening, the solution is not to prevent hot-reload out of production mode, it is to prevent the hot-reloading of mode altogether.

This means the two options that make the most sense would be:

  1. The originally intended logic, where if you start dab in dev mode we consider this a development scenario and allow you to freely hot-reload the mode so that you can quickly test how changes you are making will impact things in production mode. With this option the infiltration of dev mode specific properties of dab into the production mode makes sense because you are in a development scenario simply testing how things will work in production mode.
  2. Do not allow for hot-reloading the mode at all, and instead close down the DAB engine if any hot-reload of the mode from development to production was attempted. This prevents any possibility from having features intended for development scenarios to infiltrate into production mode, and if a developer wants to test how the changes theyve been making while in dev mode work in production mode they would have to restart the engine in production mode.

I would support the second option considering the below factors:

  1. Prevents development-specific configurations from impacting production, avoiding potential critical issues.
  2. Restarting the DAB engine ensures a fresh, isolated state for each environment, reducing errors.
  3. Supporting hot-reload between modes adds extra complexity to the logic, that may not be needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage issues to be triaged
Projects
None yet
Development

No branches or pull requests

3 participants