Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

io-wakeup: Use file descriptors when calling select. #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cbaines
Copy link

@cbaines cbaines commented Sep 11, 2023

Rather than ports, since select doesn't work for all ports in Guile.

  • fibers/io-wakeup.scm (make-wait-operation): Pass the fd to ready?.

@cbaines cbaines force-pushed the io-wakeup-always-call-select-on-fds branch 2 times, most recently from ab1f9de to f915c14 Compare September 11, 2023 11:25
@emixa-d
Copy link
Collaborator

emixa-d commented Sep 14, 2023

No. This is incompatible with ‘move->fdes’ and friends, as it is possible for a different thread to adjust the file descriptor of the port between 'port-read-wait-fd' and ‘select'. (Yes, Fibers already has this bug in fibers/epoll.scm, but that doesn't mean we should increase the number of instances of this bug.)

It also a problematic in a different way: it assumes the port has a file descriptor that can meaningfully be directly selected. While writing Scheme-GNUnet, I am intending to allow accessing CADET ‘channels’ (or whatever the terminology was ...) as ports, and they aren't even implemented as something on top of file descriptors.

Rather than ports, since select doesn't work for all ports in Guile.

Instead, why not make ‘select’ work on these ports? Also, I'm wondering which ports these are. Alternatively, if the ‘select accepts a list/vector of ports’ is difficult to implement, it would be sufficient to have something like ‘readable?’ / ‘writable?’ directly in Guile.

@emixa-d emixa-d added needs-changes This PR isn't quite right yet. question bug labels Sep 14, 2023
@emixa-d
Copy link
Collaborator

emixa-d commented Sep 14, 2023

[...] and they aren't even implemented as something on top of file descriptors.

(They aren't implemented at all yet, but that's a separate issue.)

@cbaines cbaines force-pushed the io-wakeup-always-call-select-on-fds branch from f915c14 to 9a0369a Compare September 15, 2023 12:52
@cbaines
Copy link
Author

cbaines commented Sep 15, 2023

No.

Ok.

This is incompatible with ‘move->fdes’ and friends, as it is possible for a different thread to adjust the file descriptor of the port between 'port-read-wait-fd' and ‘select'. (Yes, Fibers already has this bug in fibers/epoll.scm, but that doesn't mean we should increase the number of instances of this bug.)

That seems like a design/implementation limitation to me, not a bug.

It also a problematic in a different way: it assumes the port has a file descriptor that can meaningfully be directly selected.

I think assuming that the port has a file descriptor is fine, fibers makes that assumption elsewhere: https://github.com/wingo/fibers/blob/master/fibers.scm#L56-L63

While writing Scheme-GNUnet, I am intending to allow accessing CADET ‘channels’ (or whatever the terminology was ...) as ports, and they aren't even implemented as something on top of file descriptors.

Right, so what's the problem? If that port isn't directly blocking reading or writing on a file descriptor, then it's not relevant to fibers as it's not going to block at a low level?

Rather than ports, since select doesn't work for all ports in Guile.

Instead, why not make ‘select’ work on these ports? Also, I'm wondering which ports these are. Alternatively, if the ‘select accepts a list/vector of ports’ is difficult to implement, it would be sufficient to have something like ‘readable?’ / ‘writable?’ directly in Guile.

select in Guile works with file descriptors, at least as far as I can see with the current implementation. Maybe it could be improved by calling port_read_wait_fd or port_write_wait_fd internally, but then that's pretty much what these changes achieve.

fibers/io-wakeup.scm Outdated Show resolved Hide resolved
@emixa-d
Copy link
Collaborator

emixa-d commented Sep 17, 2023

No.

Ok.

This is incompatible with ‘move->fdes’ and friends, as it is possible for a different thread to adjust the file descriptor of the port between 'port-read-wait-fd' and ‘select'. (Yes, Fibers already has this bug in fibers/epoll.scm, but that doesn't mean we should increase the number of instances of this bug.)

That seems like a design/implementation limitation to me, not a bug.

Adding this new design/implementation limitation to io-wakeup is a bug, because nowhere does the documentation of io-wakeup state that that you must not move the fdes of the port.

I'm not opposed to adding such a limitation (AFAIK move->fdes and friends is only useful for (a) changing C's rough equivalent of current-input-port etc. (i.e. fd 0 1 2) and (b) stuff between ‘fork’ and ‘exec’, which in a multi-threaded context you can't need to do in C anyways).

It also a problematic in a different way: it assumes the port has a file descriptor that can meaningfully be directly selected.

I think assuming that the port has a file descriptor is fine, fibers makes that assumption elsewhere: https://github.com/wingo/fibers/blob/master/fibers.scm#L56-L63

This reasoning makes no sense. Just because Fibers makes an assumption somewhere doesn't mean that this assumption should be made elsewhere as well -- for ‘wait-for-readable’, there currently is no other option but to use the file descriptor and assume it has a file descriptor, whereas for ‘wait-until-port-readable-operation’, it doesn't need to make such an assumption.

While writing Scheme-GNUnet, I am intending to allow accessing CADET ‘channels’ (or whatever the terminology was ...) as ports, and they aren't even implemented as something on top of file descriptors.

Right, so what's the problem? If that port isn't directly blocking reading or writing on a file descriptor, then it's not relevant to fibers as it's not going to block at a low level?

The problem is ... what I wrote above. Whether the blocking happens at an fd level or not and at a low level or high level is ... utterly irrelevant (*)? What matters is whether it will block, not on which layer it will block -- it is, after all, wait-until-port-readable, not wait-until-fd-readable.

For a concrete example, look at https://git.gnunet.org/gnunet-scheme.git/tree/gnu/gnunet/mq-impl/stream.scm. Except for setting the FD_CLOEXEC flag, it doesn't care at all whether the ports have fds or not.

While that example uses fd-backed ports, I don't think it's hard to imagine that in the future, someone may want to wait on the readability of non-fd ports -- for example, to wait on the readability of a CADET thing. And it would be great if non-fd ports and fd ports had the same interface for this, to enable writing code that's agnostic on what kind of port it is passed.

More concretely, I intend to do that eventually in the tests of Scheme-GNUnet, to make tests have less side-effects by not creating UNIX file-system-backed sockets and not having to clean them up afterwards.

(*) except for the implementation of the operation. As it turns out that ‘select’ requires file descriptors and I don't see a good way of generalising this, I now intend to write a (separate) patch to address this.

Rather than ports, since select doesn't work for all ports in Guile.

Instead, why not make ‘select’ work on these ports? Also, I'm wondering which ports these are. Alternatively, if the ‘select accepts a list/vector of ports’ is difficult to implement, it would be sufficient to have something like ‘readable?’ / ‘writable?’ directly in Guile.

select in Guile works with file descriptors, at least as far as I can see with the current implementation. Maybe it could be improved by calling port_read_wait_fd or port_write_wait_fd internally, but then that's pretty much what these changes achieve.

This doesn't answer the question, unless this is an indirect way of stating that ‘these ports’ don't have file descriptors. But that's contradicted by the fact that your are using ‘port-read-wait-fd’ and hence the ports do, in fact have file descriptors.

If I understood correctly, it looks like you should modify 'select' to do that (in Guile), and as a temporary workaround (in Fibers) only use 'port-x-wait-fd' if the ‘(fileno port)’ returns `#false'.

emixa-d added a commit that referenced this pull request Sep 17, 2023
* fibers/io-wakeup.scm (wait-until-port-writable):
Replace readable? by writable?.

Reported-by: cbaines
See: #96 (review)
@emixa-d
Copy link
Collaborator

emixa-d commented Sep 17, 2023

Looks like io-wakeup already assumes ports have fdes (see e.g. make-read-operation/make-write-operation), which mostly does away with the concerns about new uses port-read-wait-fd and friends. Still, [...]

If I understood correctly, it looks like you should modify 'select' to do that (in Guile), and as a temporary workaround (in Fibers) only use 'port-x-wait-fd' if the ‘(fileno port)’ returns `#false'.

[...] seems to apply.

@emixa-d
Copy link
Collaborator

emixa-d commented Sep 17, 2023

(*) except for the implementation of the operation. As it turns out that ‘select’ requires file descriptors and I don't see a good way of generalising this, I now intend to write a (separate) patch to address this.

See (untested) branch fdless-draft. Might be better to use something like GOOPS (but simpler and more lightweight) here, instead of this ad-hoc stuff. Not planning on doing more with it for now.

@cbaines
Copy link
Author

cbaines commented Sep 19, 2023

I think assuming that the port has a file descriptor is fine, fibers makes that assumption elsewhere: https://github.com/wingo/fibers/blob/master/fibers.scm#L56-L63

This reasoning makes no sense. Just because Fibers makes an assumption somewhere doesn't mean that this assumption should be made elsewhere as well -- for ‘wait-for-readable’, there currently is no other option but to use the file descriptor and assume it has a file descriptor, whereas for ‘wait-until-port-readable-operation’, it doesn't need to make such an assumption.

I just don't see how or why you want to wait for reading or writing to a port that's not blocking on a file descriptor? If you implement a port in Guile that does something that's going to block, say calling some native code, then you do something there to make that cooperate with fibers. That makes perfect sense to me.

But there's no general way in Guile of magically waiting until any kind of port can be read from or written to without blocking, because it's very specific to the implementation of that port. And as above, I don't believe you need to do this, you can still write all kinds of ports that interact well with fibers.

While writing Scheme-GNUnet, I am intending to allow accessing CADET ‘channels’ (or whatever the terminology was ...) as ports, and they aren't even implemented as something on top of file descriptors.

Right, so what's the problem? If that port isn't directly blocking reading or writing on a file descriptor, then it's not relevant to fibers as it's not going to block at a low level?

The problem is ... what I wrote above. Whether the blocking happens at an fd level or not and at a low level or high level is ... utterly irrelevant (*)? What matters is whether it will block, not on which layer it will block -- it is, after all, wait-until-port-readable, not wait-until-fd-readable.

As above, I don't think this makes sense, because there's no mechanism to make it work.

If there was some way in Guile for you to hook in to ports to know how to wait on them being readable or writable, then this would be a possibility, but there isn't, the mechanism available is that ports can share the relevant file descriptors, then you do something with them.

And again, this shouldn't cause you problems if you're using a port that isn't backed by a file descriptor, but does something that blocks, as you need to cooperate with fibers internally within the implementation of the port.

For a concrete example, look at https://git.gnunet.org/gnunet-scheme.git/tree/gnu/gnunet/mq-impl/stream.scm. Except for setting the FD_CLOEXEC flag, it doesn't care at all whether the ports have fds or not.

While that example uses fd-backed ports, I don't think it's hard to imagine that in the future, someone may want to wait on the readability of non-fd ports -- for example, to wait on the readability of a CADET thing. And it would be great if non-fd ports and fd ports had the same interface for this, to enable writing code that's agnostic on what kind of port it is passed.

I don't really follow the example you give, but again, unless I've missed something massively in Guile, there is no mechanism to wait on the readability or writability of ports in general. And I don't think there needs to be.

Trying to design code in fibers around that (which I don't think exists) doesn't seem sensible.

@emixa-d
Copy link
Collaborator

emixa-d commented Sep 19, 2023

I think assuming that the port has a file descriptor is fine, fibers makes that assumption elsewhere: https://github.com/wingo/fibers/blob/master/fibers.scm#L56-L63

This reasoning makes no sense. Just because Fibers makes an assumption somewhere doesn't mean that this assumption should be made elsewhere as well -- for ‘wait-for-readable’, there currently is no other option but to use the file descriptor and assume it has a file descriptor, whereas for ‘wait-until-port-readable-operation’, it doesn't need to make such an assumption.

I just don't see how or why you want to wait for reading or writing to a port that's not blocking on a file descriptor? If you implement a port in Guile that does something that's going to block, say calling some native code, then you do something there to make that cooperate with fibers. That makes perfect sense to me.
But there's no general way in Guile of magically waiting until any kind of port can be read from or written to without blocking, because it's very specific to the implementation of that port. And as above, I don't believe you need to do this, you can still write all kinds of ports that interact well with fibers.

How: with wait-until-... and the new branch fdless-draft.
Why: I do, in fact, need to do this, see the example I mentioned.

So, I could just not wait for blocking, but the thing is that I want to wait for blocking or for another operation (e.g. a condition or channel indicating the fiber can stop, as in the example). Without an operation to wait for blocking, the fiber would could keep going even after it should be stopped.

While writing Scheme-GNUnet, I am intending to allow accessing CADET ‘channels’ (or whatever the terminology was ...) as ports, and they aren't even implemented as something on top of file descriptors.

Right, so what's the problem? If that port isn't directly blocking reading or writing on a file descriptor, then it's not relevant to fibers as it's not going to block at a low level?

The problem is ... what I wrote above. Whether the blocking happens at an fd level or not and at a low level or high level is ... utterly irrelevant (*)? What matters is whether it will block, not on which layer it will block -- it is, after all, wait-until-port-readable, not wait-until-fd-readable.

As above, I don't think this makes sense, because there's no mechanism to make it work.

See the new branch, which has the basics of making this work.

If there was some way in Guile for you to hook in to ports to know how to wait on them being readable or writable, then this would be a possibility, but there isn't, the mechanism available is that ports can share the relevant file descriptors, then you do something with them.

Again, see the new branch.

And again, this shouldn't cause you problems if you're using a port that isn't backed by a file descriptor, but does something that blocks, as you need to cooperate with fibers internally within the implementation of the port.

Again, see the new branch, which provides a mechanism for the implementation of the port to cooperate with fibers.

For a concrete example, look at https://git.gnunet.org/gnunet-scheme.git/tree/gnu/gnunet/mq-impl/stream.scm. Except for setting the FD_CLOEXEC flag, it doesn't care at all whether the ports have fds or not.
While that example uses fd-backed ports, I don't think it's hard to imagine that in the future, someone may want to wait on the readability of non-fd ports -- for example, to wait on the readability of a CADET thing. And it would be great if non-fd ports and fd ports had the same interface for this, to enable writing code that's agnostic on what kind of port it is passed.

I don't really follow the example you give, but again, unless I've missed something massively in Guile, there is no mechanism to wait on the readability or writability of ports in general. And I don't think there needs to be.

There does need to be a mechanism: see the example.
There is also a mechanism: see wait-until-... + the patch.
I never claimed there needs to be a mechanism in general -- as long as new port types have some mechanism for implementing support for wait-until-whatever, that's sufficient.

Trying to design code in fibers around that (which I don't think exists) doesn't seem sensible.

It makes sense to do that in Fibers instead of Guile, because operations are a Fibers construct, not a Guile construct.

@emixa-d
Copy link
Collaborator

emixa-d commented Sep 19, 2023

Also, if it's not considered a sensible thing, then I would say that this non-sensible thing is really useful, neat, straightforward to implement (it even has an implementation already: see the patch), desired (*) and doesn't interfere with anything else, and as such the nonsensibility is not a concern.

(*) source for the last part: me.

@emixa-d
Copy link
Collaborator

emixa-d commented Sep 19, 2023

[a general mechanism like wait-until-port-readable] And I don't think there needs to be.

In my experience, I actually need it for Scheme-GNUnet, for both current code, future code and likely future users of Scheme-GNUnet. If you want to demonstrate the contrary, go ahead and rewrite current code, write future code and write an application using Scheme-GNUnet for GNUnet stuff, I wouldn't mind some extra contributors, but insisting that this is false with only words isn't getting you anywhere.

@emixa-d
Copy link
Collaborator

emixa-d commented Sep 19, 2023

Look let's stop making things harder for my pet project (Scheme-GNUnet) and focus on making wait-until-whatever slightly more general to cover your ports, of which you still haven't answered why you don't just implement select for those ports.

Again, it would help if you actually said what those currently-unsupported ports are.

@emixa-d
Copy link
Collaborator

emixa-d commented Sep 19, 2023

But there's no general way in Guile of magically waiting until any kind of port can be read from or written to without blocking, because it's very specific to the implementation of that port. And as above, I don't believe you need to do this, you can still write all kinds of ports that interact well with fibers.

No. If wait-until-... cannot work on non-fd ports, then those ports do not interact well with fibers because for some users (e.g. Scheme-GNUnet) of those ports, being able to run wait-until-... on all kinds of ports is really convenient, see my reference to Scheme-GNUnet's tests.

Look if you will continue this as you did previously, you're failing reading comprehension and I'll be closing this PR.

@cbaines cbaines force-pushed the io-wakeup-always-call-select-on-fds branch from 9a0369a to b9c73d8 Compare September 20, 2023 13:31
Select only works for file ports or file descriptors, so pass in the
appropriate file descriptor when not using a file port.

* fibers/io-wakeup.scm (readable?, writable?): Take a port or fd.
(try-ready): Take port-ready-fd, and use this when calling ready? if
port is not a file port.
(wait-until-port-readable-operation): Pass port-read-wait-fd to
try-ready.
(wait-until-port-writable-operation): Pass port-write-wait-fd to
try-ready.
@cbaines cbaines force-pushed the io-wakeup-always-call-select-on-fds branch from b9c73d8 to 4bd3621 Compare September 20, 2023 13:37
@cbaines
Copy link
Author

cbaines commented Sep 20, 2023

I think assuming that the port has a file descriptor is fine, fibers makes that assumption elsewhere: https://github.com/wingo/fibers/blob/master/fibers.scm#L56-L63

This reasoning makes no sense. Just because Fibers makes an assumption somewhere doesn't mean that this assumption should be made elsewhere as well -- for ‘wait-for-readable’, there currently is no other option but to use the file descriptor and assume it has a file descriptor, whereas for ‘wait-until-port-readable-operation’, it doesn't need to make such an assumption.

I just don't see how or why you want to wait for reading or writing to a port that's not blocking on a file descriptor? If you implement a port in Guile that does something that's going to block, say calling some native code, then you do something there to make that cooperate with fibers. That makes perfect sense to me.
But there's no general way in Guile of magically waiting until any kind of port can be read from or written to without blocking, because it's very specific to the implementation of that port. And as above, I don't believe you need to do this, you can still write all kinds of ports that interact well with fibers.

How: with wait-until-... and the new branch fdless-draft. Why: I do, in fact, need to do this, see the example I mentioned.

So, I could just not wait for blocking, but the thing is that I want to wait for blocking or for another operation (e.g. a condition or channel indicating the fiber can stop, as in the example). Without an operation to wait for blocking, the fiber would could keep going even after it should be stopped.

Right, there's some missing information. You don't just want to interact with ports not backed by file descriptors, but you want to construct fibers operations that involve whether some port in general (including those not involving file descriptors) is readable/writable.

I guess that should be technically possible, but I don't know how it would be done. Maybe it can be done in the future by (fibers io-wakeup), but I would imagine that would involve using the existing implementation for ports involving file descriptors and using a completely different implementation internally when they're not.

Look let's stop making things harder for my pet project (Scheme-GNUnet) and focus on making wait-until-whatever slightly more general to cover your ports, of which you still haven't answered why you don't just implement select for those ports.

Again, it would help if you actually said what those currently-unsupported ports are.

The ports you get back from open-socket-for-uri when using HTTPS.

GNU Guile 3.0.9
...
scheme@(guile-user)> (use-modules (web client) (web uri))
scheme@(guile-user)> (define sock (open-socket-for-uri (string->uri "https://www.gnu.org/")))
scheme@(guile-user)> (select (vector sock) #() #() 0)
ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure select: Wrong type argument in position 1: #<input-output: string 7f61908d5c40>

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.

Recent versions of guile-gnutls do set the read and write file descriptors on the port, which enables them to be used with suspendable ports/fibers.

But there's no general way in Guile of magically waiting until any kind of port can be read from or written to without blocking, because it's very specific to the implementation of that port. And as above, I don't believe you need to do this, you can still write all kinds of ports that interact well with fibers.

No. If wait-until-... cannot work on non-fd ports, then those ports do not interact well with fibers because for some users (e.g. Scheme-GNUnet) of those ports, being able to run wait-until-... on all kinds of ports is really convenient, see my reference to Scheme-GNUnet's tests.

Look if you will continue this as you did previously, you're failing reading comprehension and I'll be closing this PR.

Up until now we've been talking about different things. I did have a go at reading the code sample you gave, but I found it difficult to follow and I didn't manage to figure out that you were trying to use the fibers operations around ports in conjunction with other operations. Now you've said you want to do that, I can understand why you want this code to work more generally.

As for your suggestion to make select in Guile use the read and write wait fds, it sounds like it might be good, and if that change happens in Guile, then that would help here.

Ignoring the part of this discussion about the hypothetical use of this code on ports not involving file descriptors, you have given useful feedback which I've incorporated. port-read-wait-fd and port-write-wait-fd are now only used when the port isn't a file port.

@emixa-d
Copy link
Collaborator

emixa-d commented Sep 24, 2023

Someone else can handle this PR, I can't anmore. @aconchillo, @wingo?

p.s. Is there a list of maintainers somewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs-changes This PR isn't quite right yet. question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants