17934 - timing bug introduced in plevent in 3.5

Status: RESOLVED WONTFIX

(SeaMonkey :: General, defect, P3)

Description

[Colin Blake]

I know the PLEvent stuff is not part of NSPR anymore, but its still in
the NSPR tree and so I'm taking the liberty of putting the bug report
into NSPR. Please reassign to the appropriate Mozilla folk.

I've run into a problem with NSPR 3.5 while trying to get M11 up and
running on OpenVMS. This is a problem that has been introduced in 3.5.

Background. I had some timing problems before on OpenVMS. Sometimes I
would go to read the char from the notify pipe, only to find it wasn't
there yet. Under this circumstance, it was important NOT to decrement
notifyCount, otherwise it would get out of whack wrt the number of
events outstanding and the number of bytes in the pipe. This fix made
it into an earlier version of NSPR.

Now I have a new problem which occurs under the same circumstances.
In this situation, we now do NOT read the char, we do NOT decrement
notifyCount, but we still return a successful status. GetEvent sees the
good status, and pulls the next event off the queue. Now the number of
pending events is less than notifyCount (and the number of bytes in the
pipe). This inconsistency didn't matter in pre-3.5, but it does matter
in 3.5 due to a change that has been made.

Assume the above has just happened and so the number of queued events is 0
but notifyCount is 1 and there's one byte in the pipe. In pre-3.5, next time
in to GetEvent we would call AcknowledgeNativeNotify. This would cause the
byte to be read from the pipe and notifyCount to be decremented. Back in
GetEvent we go to pull the event off the queue only to find there isn't one.
No problem, we just continue on our merry way HAVING JUST CORRECTED the
events/notifyCount mismatch problem.

In 3.5, GetEvent has been changed and we only call AcknowledgeNativeNotify
if the event queue has something in it. So in the above situation, we never
call AcknowledgeNativeNotify and so notifyCount stays at 1 and the byte
remains in the pipe. In the idle loop, the select keeps firing for the
pipe fd because there's a byte in there, but GetEvent will never read
the pipe because there's no events in the queue. Result is lots of CPU
being used but we get nowhere.

I see two ways to fix this.

1. Have AcknowledgeNativeNotify return false if it didn't read the byte
from the pipe. This will cause GetEvent to stall until the byte really
is ready to be read, and prevent the problem from occurring.

2. Put some code in GetEvent to detect when the queue is empty but
notifyCount is non-zero, and then suck on the pipe until the
situation is cleared.

Option 2 sound like a lot more work and needs some very careful
synchronization. Option 1 seems like the way to go. I have tried it
here on OpenVMS and it does indeed fix the problem.

Colin.

Comment 1

[srinivas]

Reassigned the bug to travis.

Comment 2

[travis]

Troy, seeing as you made these changes, I am forwarding this over to you.

Comment 3

[troy]

No, I did not make changes to GetEvent()...

Comment 4

[Colin Blake]

Attachment: patch file

Comment 5

[Colin Blake]

I just attached a patch file for option 1.

Comment 6

[don]

Travis, is this really our bug?

Comment 7

[travis]

I own this in the future.  However I don't know anything about the current
PLEvent stuff nor do I have time to really try going into it with the current
WebShell work.

Comment 8

[don]

Move to M15.

Comment 9

[don]

Move to M16 for now ...

Comment 10

[travis]

Is this still a bug/ problem?

Comment 11

[travis]

BTW, I don't own this code.  I'm not sure who to give it to if it is still a 
problem.

Comment 12

[Colin Blake]

I personally believe that this is still a problem for UNIX systems, but the 
timing is such that the bug is never revealed. It should still be fixed though!

Its not a problem for VMS any more (which used to use the UNIX code path in 
plevent) because I wrote a new native notification method for VMS, and therefore 
I no longer use the buggy code.

Comment 13

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 14

[Doug Turner (:dougt)]

taking ownership for now.  will look at later this week.

Comment 15

[Doug Turner (:dougt)]

colin is this a blocker for you? 


I have changes that I am working on that will remove only post notification once
instead of per event.  It will make this diff mute.  I am not sure that my patch
will make nsbeta3.

Comment 16

[Colin Blake]

This is no longer a blocker for me. See above, I wrote a new native notification 
method for VMS.

Comment 17

[Doug Turner (:dougt)]

futured.

Comment 18

[Doug Turner (:dougt)]

WONTFIX.