Skip to content

Implements IJobWorker.IAsyncDisposable #771

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Implements IJobWorker.IAsyncDisposable #771

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7ec802
feat: implements IJobWorker.IAsyncDisposable
xlegalles Feb 10, 2025
cc37a09
feat: PR comments. Improve disposal + UT
xlegalles Feb 11, 2025
f2b6276
fix PR comments
xlegalles Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Client/Api/Worker/IJobWorker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ namespace Zeebe.Client.Api.Worker;
/// open, the worker continuously receives jobs from the broker and hands them to a registered
/// <see cref="JobHandler" />.
/// </summary>
public interface IJobWorker : IDisposable
public interface IJobWorker : IDisposable, IAsyncDisposable
{
/// <returns>true if this registration is currently active and work items are being received for it.</returns>
bool IsOpen();
Expand Down
16 changes: 10 additions & 6 deletions Client/Impl/Worker/JobWorker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,20 @@ internal JobWorker(JobWorkerBuilder builder)
}

/// <inheritdoc />
[Obsolete("Use DisposeAsync instead.", false)]
public void Dispose()
{
_ = DisposeAsync();
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓️ should we wait here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation does not wait. This is what is problematic. But waiting an async method from a sync one is really not a good practice

Copy link
Collaborator

@ChrisKujawa ChrisKujawa Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

After reading the docs:

I thought we might want to write the methods a bit differently and make sure we close/dispose all resources more directly. Especially after the reformat we see that there several objects, we are not keeping the reference to dispose them correctly. Speaking of the _. https://github.com/camunda-community-hub/zeebe-client-csharp/blob/main/Client/Impl/Worker/JobWorker.cs#L107-L111

I guess we could simply call dispose on them, like on the Task thats starts the polling, etc.

But I guess we still need the cancelation of the source - and disposing of it

}

/// <inheritdoc />
public async ValueTask DisposeAsync()
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cancel the source token which will stop the pollingTask. Note that I've changed the log because it's not an error and very annoying to see it in reports.

source.Cancel();
// delay disposing, since poll and handler take some time to close
_ = Task.Delay(TimeSpan.FromMilliseconds(pollInterval.TotalMilliseconds * 2))
.ContinueWith(t =>
{
logger?.LogError("Dispose source");
source.Dispose();
});
await Task.Delay(TimeSpan.FromMilliseconds(pollInterval.TotalMilliseconds * 2));
logger?.LogError("Dispose source");
source.Dispose();
isRunning = false;
}

Expand Down
Loading