SCM

[#1010271] Synchronous notification contains a race condition

View Trackers | Bugs | Download .csv | Monitor

Date:
2008-02-08 02:19
Priority:
3
State:
Open
Submitted by:
Simon Haines (simonhaines)
Assigned to:
Nobody (None)
Npgsql Version:
2.0
Category:
Group:
Resolution:
None
Summary:
Synchronous notification contains a race condition

Detailed description
If there is more than one recipient of synchronous notifications, and one of these recipients performs a query on the notifying thread, the Mediator's ResetResponses() method clears the remaining notifications, raising an exception in the NpgsqlConnector.CheckNotifications() method because it is enumerating the notifications.

The Mediator's Notification event handlers should be called on their own thread.

Here is the workaround I have been using.

In the NpgsqlConnector class, add the following definition:
private class NotificationThread
{
private NotificationEventHandler handler;
private object sender;
private NpgsqlNotificationEventArgs args;
public NotificationThread(NotificationEventHandler handler,
object sender,
NpgsqlNotificationEventArgs args)
{
this.handler = handler;
this.sender = sender;
this.args = args;
}

public void threadStart()
{
try
{
handler(sender, args);
}
catch (Exception)
{ }
}
}

Then, modify the CheckNotifications method to this:
internal void CheckNotifications()
{
ArrayList ThreadList = new ArrayList();
if (Notification != null)
{
foreach (NpgsqlNotificationEventArgs E in _mediator.Notifications)
{
NotificationThread worker =
new NotificationThread(Notification, this, E);
ThreadList.Add(new Thread(worker.threadStart));
}
foreach (Thread thread in ThreadList)
{
thread.Start();
}
}
}

Works against Npgsql2.0beta2

Followup

Message
Date: 2008-08-11 01:45
Sender: Simon Haines

For multiple notifications per query, yes you would lose
normal ordering. If normal ordering is required, a single
thread can can be used to fire all notifications in order,
instead of multiple threads firing one notification
out-of-order.

I haven't looked at this code for some time, but it seems to
me from looking at it now that the problem is that the
original notification list was not protected from client
actions--my patch seems to have compensated for this by
moving the notification list into thread-local storage.
Specifically, the following situation could occur:
1. Npgsql events are triggered
2. Npgsql enumerates events
3. Npgsql notifies client(s) with first event
4. Client calls npgsql in query
5. Npgsql clears event list as part of query
6. Client finishes query, npgsql returns to step 3 for next
event, list was cleared in step 5, exception occurs.

By making the enumerable list thread-local, it didn't matter
if the list was cleared. I think you could achieve the same
effect without threads by copying the event list to the
stack before enumerating it. Something like:
if (Notification != null)
{
List<NpqsqlNotificationEventArgs> notificationEvents =
new
ArrayList<NpqsqlNotificationEventArgs>(_mediator.Notifications);
// Note copy
foreach (NpgsqlNotificationEventArgs E in
notificationEvents)
{
Notification(this, E);
}
}

I haven't tested this code: it's just a rough idea to
illustrate a point. I don't think its acceptable to simply
say 'don't query on a notification' because a main feature
of notifications is that distributed clients can know when a
table is updated, and thus requery its contents, in a
lightweight way (i.e. without polling). Thus I believe
querying on a notification is a natural thing to want to do.

The issue of a client blocking on a notification is more
serious, as that prevents notification delivery to all other
clients. In light of this, I believe the real solution is to
create a separate thread for each notification listener and
deliver all messages (in a local copy of the list) to that
listener in order on that thread. Thus if a single listener
blocks, all other clients still receive their notifications
in order, and because the list is thread-local, the
notification listeners are not stepping on eachother's data
structures. I don't know how thread-safe the rest of npgsql
is though.
Date: 2008-08-10 07:41
Sender: Francisco Figueiredo jr.


Correct me if I'm wrong, but one possible issue with this approach is that you can loose the actual order of the notifications as there is no way of telling what will be the order the threads will be executed.

Any ideas about how to solve that?
Unless we create another thread to keep calling join on each notification thread so the order is guaranteed.

This way, I think we could get much more complex code. We could simplify by putting in the manual that notification handlers should not execute other commands nor lock.
Date: 2008-05-22 03:20
Sender: Carlos Orrego

Forgot to add, I'm using npgsql 1.1 (or 1.0.1.. the thing that comes after 1.0) and it worked without any issue.
Date: 2008-05-22 03:18
Sender: Carlos Orrego

Hi Simon, I was using notifications too, and I was losing some notifications. After some research, I found out that they were just delivered later along with the next notification. I applied your patch and it seems to work fine now, but I'm still testing. I'll let you know if it solves my problem too.
Anyway, thanks a lot!

Attached Files:

Changes:

No Changes Have Been Made to This Item

Powered By FusionForge