pipefs doesn't handle close of last reader(/writer?) correctly

[jackburton]

I tried to compile bash 3.2 under haiku. The compiling process hangs to a point where a script tests the pipe limits.

Attached there is the (slightly modified to be able to build it standalone) program + script for testing.

Compile the c file with the name psize.aux and run the psize.sh script. Expected result: It should print the maximum pipe size after a while What happens: it never stops.

[bonefish]

​http://www.opengroup.org/onlinepubs/009695399/functions/write.html mentions nothing of sending a SIGPIPE, when the pipe buffer is full. I also don't think this would be the correct behavior.

The way, I believe, the script is supposed to work, is that after 3 seconds (i.e. when "sleep 3" exits) the read end of the pipe is closed, which then should raise a SIGPIPE, and cause the program to write the number of bytes written to the pipe to stderr.

It looks like our pipefs doesn't unblock writers when the last reader of a pipe has has gone.

[jackburton]

Replying to bonefish:

​http://www.opengroup.org/onlinepubs/009695399/functions/write.html mentions nothing of sending a SIGPIPE, when the pipe buffer is full. I also don't think this would be the correct behavior.

I see. I just misinterpreted the script, then :)

The way, I believe, the script is supposed to work, is that after 3 seconds (i.e. when "sleep 3" exits) the read end of the pipe is closed, which then should raise a SIGPIPE, and cause the program to write the number of bytes written to the pipe to stderr.

That makes sense, indeed.

It looks like our pipefs doesn't unblock writers when the last reader of a pipe has has gone.

Should we change the description of the bug ?

[jackburton]

I tried to delete the semaphores on Close(), in case the last reader quits. This way the script works correctly, but I don't know if the patch is correct, maybe we should just interrupt the semaphore acquisition ?

[bonefish]

Replying to jackburton:

I tried to delete the semaphores on Close(), in case the last reader quits.

I don't think this suffices. The writers will return with the wrong error code. Moreover they might need to send a SIGPIPE, if they haven't read anything (needs to be verified). The same goes for readers, I guess.

This way the script works correctly,

I think, it does only, because the writer keeps trying to write. So after returning with B_BAD_SEM_ID, it tries again and then a SIGPIPE is sent.

but I don't know if the patch is correct, maybe we should just interrupt the semaphore acquisition ?

If you mean sem_interrupt_thread(), that wouldn't work since the write threads aren't known at this point -- that could be changed, of course -- and that would introduce a race condition (the thread could be interrupted before trying to acquire the sem in the first place).

If you mean release_sem(), that would at least require some additional handling on the writer part.

But to be honest, I don't like the whole reader/writer interaction. The TTY code is much nicer although it has more complex requirements. I suppose completely redoing that part of the pipefs implementation would be the best option.

[jackburton]

Replying to bonefish:

Replying to jackburton:

I tried to delete the semaphores on Close(), in case the last reader quits.

I don't think this suffices. The writers will return with the wrong error code. Moreover they might need to send a SIGPIPE, if they haven't read anything (needs to be verified). The same goes for readers, I guess.

This way the script works correctly,

I think, it does only, because the writer keeps trying to write. So after returning with B_BAD_SEM_ID, it tries again and then a SIGPIPE is sent.

Yeah, I understand. But isn't that the point ? I mean, arent' we trying to get that signal to the thread ? Of course, if the pipe can be opened again later (I have no idea about pipes semantics), that approach won't work correctly.

but I don't know if the patch is correct, maybe we should just interrupt the semaphore acquisition ?

If you mean sem_interrupt_thread(), that wouldn't work since the write threads aren't known at this point -- that could be changed, of course -- and that would introduce a race condition (the thread could be interrupted before trying to acquire the sem in the first place).

I see.

If you mean release_sem(), that would at least require some additional handling on the writer part.

Indeed. Using release_sem() is the first thing I tried, but of course it doesn't work unless you add more code, as you said.

[bonefish]

Replying to jackburton:

Replying to bonefish:

Replying to jackburton:

This way the script works correctly,

I think, it does only, because the writer keeps trying to write. So after returning with B_BAD_SEM_ID, it tries again and then a SIGPIPE is sent.

Yeah, I understand. But isn't that the point ? I mean, arent' we trying to get that signal to the thread ?

Yep, but ideally we do that according to the standard. Letting the write() fail with B_BAD_SEM_ID, hoping that the caller will retry is definitely not the way it should work.

Of course, if the pipe can be opened again later (I have no idea about pipes semantics), that approach won't work correctly.

No, the pipe can't be opened again. Once all users of one end have closed it, the pipe is unusable.

[bonefish]

Fixed in hrev22378.