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 #3838 - Schedule Jobs looping after new install #3856

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

sbwalker
Copy link
Member

No description provided.

@sbwalker sbwalker merged commit 06b2eb7 into oqtane:dev Feb 20, 2024
1 check passed
@thabaum
Copy link
Contributor

thabaum commented Feb 20, 2024

@sbwalker This does fix #3838, however maybe we need to log another issue relating to deleting the scheduled jobs maybe should not be allowed (delete button inactive) since once you uninstall them, the next time you restart Oqtane these scheduled jobs are installed again. Or would these jobs being added again if not present be the expected behavior?

I suppose I am inquiring about the current behavior, to review if another issue is present or not? If these two scheduled jobs are expected to always be installed then moving on, unless they should be not meant to be deleted/reinstalled.

@sbwalker
Copy link
Member Author

@thabaum you are correct, there should not be a delete button (similar to the fact that there is no Add button). Scheduled jobs are automatically added on startup if the assembly exists in the /bin.

@zyhfish
Copy link
Contributor

zyhfish commented Feb 21, 2024

we still need to find a complete fix for this infinite loop issue, as it may also occur in other controls.

@sbwalker
Copy link
Member Author

sbwalker commented Feb 21, 2024

@zyhfish it should not occur in other components if those components are developed correctly. Basically this is the same as calling StateHasChanged in an OnParametersSet event - it will result in an infinite loop in Blazor.

@zyhfish
Copy link
Contributor

zyhfish commented Feb 21, 2024

Thanks @sbwalker , these controls should need to be updated as they called AddModuleMessage in OnParametersSet method:
Oqtane.Client\Modules\Admin\Files\Index.razor
Oqtane.Client\Modules\Admin\Logs\Index.razor
Oqtane.Client\Modules\Admin\ModuleDefinitions\Index.razor
Oqtane.Client\Modules\Admin\Themes\Index.razor
Oqtane.Client\Modules\HtmlText\Index.razor

I will try to analyze the possible calls to StateHasChanged in OnParametersSet by any other functions to check whether has other controls need to be updated, then submit a PR to handle them.
Thanks,

@sbwalker
Copy link
Member Author

sbwalker commented Feb 21, 2024

@zyhfish the components you identified are indeed problematic and could result in infinite loops if an exception occurred in the OnParametersSet method:

ie.

    protected override async Task OnParametersSetAsync()
    {
        try
        {
            // do some stuff
        }
        catch (Exception ex)
        {
            await logger.LogError(ex, "Error {Error}", ex.Message);
            AddModuleMessage("Error", MessageType.Error); // this will call OnParametersSet resulting in an infinite loop
        }
    }

Most likely the best approach is to remove the AddModuleMessage() method call from the exception handler in these methods. The user will not get any feedback that an error occurred - but it will prevent an infinite loop. Inifinite loops are especially problematic in this case because the logger.LogError() generates Notifications which are sent by email. So your email box could get spammed with hundreds of messages as a result of an infinite loop.

@sbwalker
Copy link
Member Author

@zyhfish PR #3875 resolves the issues in the components you identified

@zyhfish
Copy link
Contributor

zyhfish commented Feb 24, 2024

Hi @sbwalker , thanks so much for the update, I also submitted a PR #3885 to avoid this issue, now we can calling AddModuleMessage in the OnParameterSet method without infinite loop issue, please check whether this is ok. if ok we can revert #3875 as there is no feedback on UI if any exception occur with this change.

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.

3 participants