search

marcoCasamento / Hangfire.Redis.StackExchange

HangFire Redis storage based on original (and now unsupported) Hangfire.Redis but using lovely StackExchange.Redis client
Other
460 stars 109 forks source link

BUG: Job queue is always default in history #138

Closed Lexy2 closed 6 months ago

Lexy2 commented 6 months ago

Steps to repro:

  1. Enqueue a job for a queue other than default.
  2. Optionally, execute it by a server.
  3. Query the monitoring API for the job status history.

Expected: History data contains "Queue": "<queue-name>"

Actual: History data contains "Queue": "default"

marcoCasamento commented 6 months ago

That's interesting and it seems clearly linked to #142. All of the monitoring API relies on GetJobsWithProperties to fetch history data, do you find the problem with all methods ? (FailedJobs, SucceededJobs, ProcessingJobs etc)

Lexy2 commented 6 months ago

Nope, this is just for ScheduledJobs. The reason is that other states have implemented handlers in the storage. I'll have a crack at it, the problem seems a bit deeper.

Lexy2 commented 6 months ago

Actually, misread your comment. All the monitoring API methods do not return the queue. It's only mentioned in history. It may be linked, yes.

marcoCasamento commented 6 months ago

We agree that Job.Queue is where the queue is supposed to be and static Job RedisMonitoringApi.TrytoGetJob is used to return the job. In turn, that method only take string type, string method, string parameterTypes, string arguments and passes these values to the overload of InvocationData constructor that doesn't take a Queue as input.

If we use the cosntructor that take the queue, the corresponding Queue prop of the job would be valorized with passed queue value.

So the problem boils down about how to pass that argument to the InvocationData constructor. In turns, it depends on the values that are loaded from Redis, and here I was wrong, those are not loaded only through "GetJobsWithProperties" but also through "ScheduledJobs" and "JobDetails". Basically the three reference of RedisMonitoringApi.TryToGetJob.

I believe we should add a string queue parameter to this method, modify it to pass that argument to the InvocationData constructor and above all modify the three reference of RedisMonitoringApi.TryToGetJob so to pass the correct value for the new arg. Definitely a bug.

Lexy2 commented 6 months ago

Apparently, this particular issue is not related to #137 or #142. The "Queue": "default" hash entry comes from the primary EnqueuedState constructor - see Hangfire/EnqueuedState.cs, and is set to default regardless of what's specified in the job. However, this value is used if the job does not specify a queue (i.e. storage doesn't support Queue property, or Enqueue is called without specifying the queue explicitly).

We could potentially implement an EnqueuedStateHandler that would strip this property from the state to avoid confusion. But I'm not sure how this is used in the wild - maybe QueueAttribute depends on it and such? With the fix of #137 I'm happy to just ignore this issue.