Yes good point as that setting does affect what is stored pretty significantly. Changing the setting at a global level like that also adds a fair amount of overhead to the stored InvocationData as it changes everything so would be good to not have to do that.
Without looking at the code it seems that the Arguments are stored as a list of ParameterTypes and list of Arguments. Is the solution to ensure that the list of ParameterTypes is the runtime type of the argument, not the static type, and then use that when deserialising the Arguments array? That way existing jobs do not change, they already have the static type defined, it will only affect new jobs?
The fact InvocationData already stored as a single JSON object though suggests no further deserialisation happens after JSON converts it to a list of objects?
Type, MethodName and ParameterTypes are used to clearly identify the corresponding MethodInfo. Maybe we can remove the ParameterTypes, use TypeNameHandling.All to persist the argument types and use GetType methods of arguments to identify a method at runtime. However, we need to investigate the extreme cases, i.e. when this will not work.
I’m not planning to make such an investigation, however you could help me with that. What do you think?
We have a case related to serialization policy too. We want to keep type information in the Success state result to deserialize easily not knowing the particular result type (some generic code that could do just FromJson<object>). The only extension point exists now is JobHelper.SetSerializationSettings. But it affects serialization policy globally which is not desiarable, instead would be great if we could just override how state result is serialized.
Happy to help how I can as I would like to have a decent solution for this instead of patching over the problem. Any pointers for where to start investigating this?
@antrax, I’m in doubt that Hangfire will support fine-grained serialization policies. What if instead on relying on a Succeeded state’s result, you put it to your database instead? Succeeded state result in my imagination is needed only for debugging and informative purposes (i.e. only in dashboard).
@AdamBarclay, there are always two main questions, and some sub-questions we may start with:
What are the benefits of this feature – why do we really need it? (love this question )
How this feature or its implementation can beat us?
Will it increase the payload. If yes, how much?
What serialized data current users may have?
Can we gracefully handle all the serialized data (from the question above) with new defaults?
Can we remove the ParameterTypes data, without losing the precise method matching (type, method name and parameter types clearly define the the target method)?
Can we remove the JobHelper.SetSerializerSettings method?
@odinserj OK I’ll give answering these questions a go:
To me the benefits of making this change is to ensure that, OOTB, Hangfire will ‘just work’ with common code constructs, in this case a polymorphic call that fails unless a (undocumented?) setting is changed. A setting that probably makes sense to be internal or very rarely modified to avoid unexpected and untested bugs.
Changing the JSON setting to TypeNameHandling.All also affects all data storage which introduces extra overhead of the payload, far more than if this was a targeted change to argument types.
2.1 Yes the payload will increase because of the extra type information required to be stored for the arguments. The increase depends on the exact assembly name, number of parameters etc. Given a long assembly / class name and 5 parameters (likely to be above average) I would expect an increase of ~700 characters of JSON data.
An alternative would be to store just a list of types in a new column / property (e.g. RuntimeArgumentTypes). This decreases additional overhead to ~630 characters.
This is a worst case scenario though. Primitive types such as string or bool could be stored in fewer characters.
2.2/2.3 The current serialised data is going to be the plain JSON objects within InvocationData / Arguments (unless previously modified to use TypeNameHandling.All). There are likely to be some complications with how this current JSON could be deserialised if we just change the default here.
2.4 This probably requires more thought about the language specification and what is legal. I think it probably makes sense to keep this information so we know exactly what method to use given a number of overloads etc. Let the compiler figure out this information for us?
2.5. Because it has such a large affect on the storage of data it probably makes sense for it to not be exposed. That being said it depends on what it has already been used for by users? Is it for this situation?
To handle a number of concerns would it make sense that this is developed by adding additional properties / columns instead of attempting to migrate existing data? So if new prop exists we can use a more accurate type information, else the existing mechanism.
I mentioned having an array of runtime types stored could help, but thinking about it more that is problematic if the argument data is still stored without types as deserialising through JSON.NET would produce object instances anyway (not sure if we could hook in to the logic?) s InvocationData is a whole JSON graph of the data.
Right now without knowing the code and how parts are used the answers may be a little hazy.
We consider Hangfire database as our jobs storage, consequently theirs results too. Putting it in our own db is fine unless you don’t want to sync 2 databases every time some change is occurred (e.g. job deleted). Hangfire provides JobParameters which is a nice feature allowed us to attach custom information to jobs and eliminate the need to keep that in the application database. I don’t think fine-grained serialization policy is needed, the challenge can be solved by providing some hook to current IState implementations, e.g. if I could customize SucceededState behavior (like override SerializeData method). With 1.5 I could do that in a some lengthy way by having custom IStateMachine where I can decorate SucceededState with custom behavior on serialization.
)