Fix distributed deadlock between two sessions

This code gets rid of holding a lock during the call to Send(). In general, it is a good idea not to hold locks while making potentially blocking operations.

I've seen the other pull request, which removes the lock when reading responder_ in HasResponder.

I think that is not correct solution: the responder_ member is a mutable property in Session, and as such needs some kind of synchronization for access. There is no real argument that this would work more than observing the fact that it doesn't seem to cause problems.

Another problem is that the Disconnect() method is also protected by the same monitor, and this patch makes sure that calls to Disconnect also succeed without having to wait for a long Send() to return.

At last, this is what the code in quickfixj does.
This is still holding a same lock on the entire I/O operation – so that’s no change compared to original situation.

A separate dequeuing thread would be needed.

 

I would keep the original SendRaw method as it was – so that people can continue using it in existing code.

I would then rename method with your original fix to something like SendRawUnsynchronized – so that users can build their own queuing mechanism around it (e.g. BlockingQueue, NDistrurptor etc.). Low level messaging code shouldn’t be doing those higher level tasks IMHO.

 

Jan

 

From: staffanu [mailto:notifications@github.com] 
Sent: Friday, November 21, 2014 8:17 PM
To: connamara/quickfixn
Cc: jakrivan
Subject: Re: [quickfixn] Fix distributed deadlock between two sessions (#287)

 

In QuickFIXn/Session.cs:

>              }
> +            this.Log.OnOutgoing(message);
> +            return responder.Send(message);

Agreed. I think this is wrong too, unfortunately. In our system there is one thread that sends messages off a queue (since there is no send queue in quickfixn), so messages are serialized correctly, but it might not be in a multi threaded application.

Anyway, thinking a bit about solutions to this I currently see two solutions:

1.      (As in quickfixj): Have a queue for outgoing messages, with a separate thread that writes to the socket.

2.      Introduce a new lock around a small queue for outgoing messages in Session. We could use the thread that calls Send() to send the next message from the queue instead of sending the current message, in order to ensure correct message order.

I implemented 2 in https://github.com/staffanu/quickfixn/compare/fix-deadlock-fix but didn't make a pull request since I'm not really convinced this is the correct solution. Comments?

Staffan

—
Reply to this email directly or view it on GitHub <https://github.com/connamara/quickfixn/pull/287/files#r20735215> .  <https://github.com/notifications/beacon/ADofNM6wS1kH1Go2GSxmn8OntMs7U82bks5nP4cmgaJpZM4C3cS_.gif>