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.
IMHO you cannot achieve what you want without a separate – queue worker – thread.

You used lock ‘B’ for I/O operations and locks ‘A’ and ‘B’ for inserting into the queue and lock ‘A’ is also used for receiving operations.

As a result if ‘B’ is held and blocked due to a long I/O, ‘A’ will be eventually also blocked (once other sender enters ‘A’ and then will need to wait for ‘B’). So the very same situation happens just one message sending call later.

 

Removing lock from HasResponder is a viable solution, since currently it is manipulated by the reading thread anyway, so it wouldn’t get a stale value. I agree it is not the best solution from the academic coding standards point of view – but QuickFixN itself isn’t the best example of good coding standards (especially for perf dependend code). So I’d simply stick with simplest working solution.

 

Jan

 

From: staffanu [mailto:notifications@github.com] 
Sent: Monday, November 24, 2014 4:03 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);

I agree that opening up for the application to take care of a send queue, if necessary, is probably a good idea. It would be necessary to provide an easy to understand way to generate and use sequence numbers, however.

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.

It is different in that the lock is held only while sending messages, and never taken otherwise. This means we can still receive messages and close the connection while a send operation is in progress.

The problem with the original code is that it deadlocks in high traffic situations.

Staffan

—
Reply to this email directly or view it on GitHub <https://github.com/connamara/quickfixn/pull/287/files#r20794426> .  <https://github.com/notifications/beacon/ADofNFgH5H21LvnaClfqeZRS5bld1peiks5nQ0AugaJpZM4C3cS_.gif>