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

HttpBackgroundJob helper component #12009

Merged
merged 3 commits into from
Jul 26, 2022
Merged

HttpBackgroundJob helper component #12009

merged 3 commits into from
Jul 26, 2022

Conversation

jtkech
Copy link
Member

@jtkech jtkech commented Jul 14, 2022

For now this component is not used at all in our source code.

It allows to execute a background job in an isolated ShellScope after the current HTTP request is completed. It allows a controller (or razor page) action to return without waiting for a job to complete, and then, if the job update some state data, also allows to render the job state.

And just a copy paste from ModularBackgroundService to make 2 or 3 helper extensions to not have duplicate code.


internal static class ShellContextExtensions
{
public static HttpContext CreateHttpContext(this ShellContext shell)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static HttpContext CreateHttpContext(this ShellContext shell)
private const string Localhost = "localhost";
private static readonly char[] _urlHostSeparators = new[] { ',', ' ' }
public static HttpContext CreateHttpContext(this ShellContext shell)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay done.

var context = new DefaultHttpContext().UseShellScopeServices();

context.Request.Scheme = "https";
var urlHost = settings.RequestUrlHost?.Split(new[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries).FirstOrDefault();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var urlHost = settings.RequestUrlHost?.Split(new[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries).FirstOrDefault();
var urlHost = settings.RequestUrlHost?.Split(_urlHostSeparators, StringSplitOptions.RemoveEmptyEntries).FirstOrDefault();

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay done.


context.Request.Scheme = "https";
var urlHost = settings.RequestUrlHost?.Split(new[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries).FirstOrDefault();
context.Request.Host = new HostString(urlHost ?? "localhost");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
context.Request.Host = new HostString(urlHost ?? "localhost");
context.Request.Host = new HostString(urlHost ?? Localhost);

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay done.

using OrchardCore.Environment.Shell.Builders;
using OrchardCore.Modules;

namespace OrchardCore.BackgroundTasks;
Copy link
Member

Choose a reason for hiding this comment

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

Can we use namespace block for consistency with the entire source

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, no closed opinion but for now waiting for other feedbacks.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a topic for #10724
For now, we should keep brackets for consistency.
Else, we can plan to do a code cleanup from VS with this new coding style and change them all if we decide so.

// Wait for the current 'HttpContext' to be released with a timeout of 60s.
while (httpContextAccessor.HttpContext != null)
{
await Task.Delay(1_000);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await Task.Delay(1_000);
await Task.Delay(1_000);

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the difference?

Copy link
Member

Choose a reason for hiding this comment

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

Just a new line before the considtion ;)

/// <summary>
/// Executes a background job in an isolated <see cref="ShellScope"/> after the current HTTP request is completed.
/// </summary>
public static Task AfterEndOfRequestAsync(string jobName, Func<ShellScope, Task> job)
Copy link
Member

Choose a reason for hiding this comment

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

ExecuteAfterEndOfRequestAsync() or RunAfterEndOfRequestAsync

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, just to not be too long, the usage being

HttpBackgroundJob.AfterEndOfRequestAsync()

In place of

HttpBackgroundJob.ExecuteAfterEndOfRequestAsync()

But why not, no closed opinion but for now waiting for other feedbacks

Copy link
Member

Choose a reason for hiding this comment

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

We could rename the class to HttpBackgroundJobRunner, but again using verbs for methods is a best practice

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I will think about it and then change the name ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay done

Copy link
Member

@hishamco hishamco left a comment

Choose a reason for hiding this comment

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

LGTM except using namespace block everywhere just for consistency, but not a big issue ;)

@agriffard agriffard merged commit 3f0fdb1 into main Jul 26, 2022
@agriffard agriffard deleted the jtkech/background-job branch July 26, 2022 22:26
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.

4 participants