fixing hanging stdin read in the switchboard #46
Conversation
miki725
changed the title
the issue we are trying to solve is:
```
echo foo | bar --stdin
```
If bar is reading its input from stdin, stdin needs to close
so that bar can proceed. If stdin is not closed, bar is hang
waiting for the stdin.
In libc if stdin is closed we get a `read()` output of 0
after a read which returned some data:
```
read("foo\n") -> 4
read("") -> 0 // as stdin closed
```
if the stdin is not closed, the second read will hang forever
as it will wait for the stdin to close
What the fix does here is that when it sees a read() returning 0
it passes that indication to all the subscribers to let them know
that the write FD should be closed to mimick what we got on the input
side of the switchboard.
Also it fixes some mixed whitespace as things werent rendering well
in vim.
|
Nice work on fixing this. But can we please:
It's straightforward to review this PR without the whitespace changes (well, at least locally; GitHub hides the option somewhat), but a commit should be a single logical unit of change. Somebody who looks at this PR or the merged commit later shouldn't be forced to hide whitespace changes in order to understand the diff. A PR may be multiple logical units of change, but then it should be multiple commits that are preserved (that is, without squashing) on merging to the main branch. |
as such needed to add `close_fd_when_done` to `fd_party_t`
|
as there is no de-facto autoformatter for C I manually mixed some of the mixed whitespace in C files I had to debug to fix this bug. splitting that out will be much more pain than provide benefit. we can add CI validation for that in the future change. there is more mixed whitespace in the repo |
There was a problem hiding this comment.
I think the work is solid, just not where I think the API change should be made, per my other comment.
The subscription object should track, and the publish needs to consult that before propagating (whereas the typical message gets broadcast to all subscribers).
Possibly it's best to have a separate version of publish for when this situation occurs, to keep it down to a single test per message (and that test is both fine where you have it, and was no net additional cost).
I'm 100% fine if you don't want to make that change, but if not, I'd prefer only addressing this for your current use case, and I can come in and make the change later. Or, you can continue to overachieve, that's fine too :)
nimutils/c/switchboard.c
Outdated
| @@ -231,31 +232,32 @@ sb_init_party_fd(switchboard_t *ctx, party_t *party, int fd, int perms, | |||
| fd_obj->first_msg = NULL; | |||
| fd_obj->last_msg = NULL; | |||
| fd_obj->subscribers = NULL; | |||
| fd_obj->close_fd_when_done = close_when_done; | |||
There was a problem hiding this comment.
As I'm taking a first scan of the code, I'm confused.
Is this flag going to cause the fd actually closing on its own lead to us PROPAGATING the close? Because:
- If so, the name isn't clear.
- I don't think this should be done per data source, I think it should be done per SUBSCRIPTION.
As just a real simple example, if I'm multiplexing a shell, and the user's stdin disappears, we established yesterday that I probably don't want to let the child shell know one of the parents is gone. HOWEVER, if that multiplexing is ALSO capturing an input log with a second subscription, I do want to know about that.
There was a problem hiding this comment.
I guess I'd call the flag something like proxy_close at the subscription level, and proxy_stdin_close at the runCmd level?
Really that's the issue, controlling whether to propagate closes for subscriptions (proxy is probably both clearer to more people and more concise?)
There was a problem hiding this comment.
ah right makes sense. Ill adjust. thanks
by configuring each subscriber, whether it should be closed when the input proxy is closed, will allow for multiplex-type application. otherwise when the input fd is closed and all subscribers are closed unilaterally is not necessarily the wanted behavior in all cases
the issue we are trying to solve is:
If bar is reading its input from stdin, stdin needs to close
so that bar can proceed. If stdin is not closed, bar is hang
waiting for the stdin.
In libc if stdin is closed we get a
read()output of 0after a read which returned some data:
if the stdin is not closed, the second read will hang forever
as it will wait for the stdin to close
What the fix does here is that when it sees a read() returning 0
it passes that indication to all the subscribers to let them know
that the write FD should be closed to mimick what we got on the input
side of the switchboard.
Also it fixes some mixed whitespace as things werent rendering well
in vim.