connamara / quickfixn

QuickFIX/n implements the FIX protocol on .NET.
http://quickfixn.org
Other
470 stars 558 forks source link

Problem when debugging - Message.ToString() alters state, which does weird things with VS debugger #863

Closed baffled closed 2 weeks ago

baffled commented 3 months ago

When debugging in e.g. Visual Studio - The Message.ToString() override creates and appends the checksum (tag 10) to the trailer. This means if you are watching the message, stepping through and VS is showing the message, or setting a break point and getting the hover, the tag is always added to the message... so when it gets to add the real checksum, that throws a repeated tag exception, which is confusing.

gbirchmeier commented 3 months ago

I agree that Message.ToString() is not properly following the inheritance guidelines (specifically, it alters the internal state). I don't like it, but changing it now would be disruptive, so it merits careful consideration.

However, I don't see how it could add a checksum twice. It doesn't "append" the checksum to the trailer; it sets the field. If you call Message.ToString() a hundred times, then it won't add 100 checksums, but it will overwrite the the trailer's checksum value 100 times.

So I'm not sure if you are accurately conveying your situation. You aren't manually setting the checksum at any point, are you?

baffled commented 3 months ago

Hi Grant

Thanks for getting back on this so quickly.

The issue arises when it is parsing an incoming message that it has received and that is hitting the Message.FromString() overload.

Imagine I am running through Visual Studio with a break point somewhere (in this case working out what was happening with SetGroup and the other issue I reported), or stepping through the method – VS will helpfully fill out its autos list with all the variables in the local context including ‘this’ pointing to the current Message instance. In order to render it VS calls the Message.ToString() that then appends (and subsequently changes) the checksum tag before it has parsed the whole message. Or I might choose to inspect it some other way which would have the same effect.

Since it’s picking up a received message and will therefore need to add its received checksum to test it for validity, that’s the point when it hits the repeated tag issue.

For now I’ve done a quick and ugly hack to get through it but thought it worth mentioning in case anyone else hits it as it wasn’t the most obvious thing to debug.

/*if (!this.Trailer.SetField(f, false))
   this.Trailer.RepeatedTags.Add(f);*/

if (!this.Trailer.SetField(f, (f.Tag == Tags.CheckSum)))
   this.Trailer.RepeatedTags.Add(f);

Kind Regards

Brian

gbirchmeier commented 3 months ago

Ah, on message reception... I think you might be on to something. I don't see it yet, but I'll dig into it.

I don't understand your hack. Why do you have code setting fields on the Trailer? And why would you ever modify Trailer.RepeatedTags in user code?

gbirchmeier commented 2 weeks ago

I didn't initially want to, but now I'm attempting it:

I'm seeing if I can

  1. rename the existing Message.ToString() and redirect existing usages to the new name
  2. re-implement ToString() so it obeys the inheritance contract

I've been putting breaking changes in like crazy this year, so what's one more.

baffled commented 2 weeks ago

Thanks, Grant – I’m sorry to hit you with this.

It would have been useful were DebuggerBrowseable applicable to ToString() or there were a Debugger.IsReallyJustTheObjectInspector property but any other workaround I can think of is likely to be more confusing or at the least, not future-proof.

Best Regards and truly appreciate all your efforts.

Brian

gbirchmeier commented 2 weeks ago

The problem is, I don't fully understand the issue that you ran into. Going back over, it really reads like ToString was a core of your problem. If that's not true, then I'm really missing it. And the part about your hacks really didn't make any sense to me, and you didn't respond to my followup about that.

I'm open to continuing to fix things, but you're going to need to walk me through it, or give me something I can run locally to replicate.

baffled commented 2 weeks ago

Hi Grant

It’s been a while since I looked but yes, ToString is the core of the problem. Inspecting an object should not change its state, which is what happens when it appears in the Locals tab when stepping through or you hover over it when debugging, since both of course invoke ToString on the object.

I can’t remember what I said about my hacks, other than that I have to customize it to my client which is down to me and only because they are dealing with third parties that don’t obey the FIX standard. Apols if I didn’t respond, as I said in a previous email I’m now away from that project until they decide they need me again. I give it six months 😊.

Go well,

Brian