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

Update for Janet 1.17 #1

Closed
subsetpark opened this issue Sep 1, 2021 · 4 comments
Closed

Update for Janet 1.17 #1

subsetpark opened this issue Sep 1, 2021 · 4 comments

Comments

@subsetpark
Copy link

Janet 1.17 deprecates the old thread interface, meaning watchful fails to compile:

In file included from src/watchful/fse.c:1:
src/watchful/../watchful.h:77:3: error: unknown type name 'JanetThread'
   77 |   JanetThread *parent;
      |   ^~~~~~~~~~~
src/watchful/inotify.c: In function 'handle_event':
src/watchful/inotify.c:98:9: warning: implicit declaration of function 'janet_thread_send' [-Wimplicit-function-declaration]
   98 |         janet_thread_send(stream->parent, janet_wrap_pointer(event), 10);
      |         ^~~~~~~~~~~~~~~~~
src/watchful.c: In function 'cfun_watch':
src/watchful.c:350:25: warning: implicit declaration of function 'janet_thread_receive' [-Wimplicit-function-declaration]
  350 |         int timed_out = janet_thread_receive(&out, timeout);
      |                         ^~~~~~~~~~~~~~~~~~~~
In file included from src/watchful/inotify.c:1:
src/watchful/../watchful.h:77:3: error: unknown type name 'JanetThread'
   77 |   JanetThread *parent;
      |   ^~~~~~~~~~~
src/watchful/inotify.c: In function 'handle_event':
src/watchful/inotify.c:98:9: warning: implicit declaration of function 'janet_thread_send' [-Wimplicit-function-declaration]
   98 |         janet_thread_send(stream->parent, janet_wrap_pointer(event), 10);
      |         ^~~~~~~~~~~~~~~~~
In file included from src/watchful.c:1:
src/watchful.h:77:3: error: unknown type name 'JanetThread'
   77 |   JanetThread *parent;
      |   ^~~~~~~~~~~
src/watchful.c:94:8: error: unknown type name 'JanetThread'
   94 | static JanetThread *watchful_current_thread() {
      |        ^~~~~~~~~~~
src/watchful.c: In function 'watchful_current_thread':
src/watchful.c:96:13: error: 'JanetThread' undeclared (first use in this function); did you mean 'JanetStream'?
   96 |     return (JanetThread *)janet_unwrap_abstract(cfun(0, NULL));
      |             ^~~~~~~~~~~
      |             JanetStream
src/watchful.c:96:13: note: each undeclared identifier is reported only once for each function it appears in
src/watchful.c:96:26: error: expected expression before ')' token
   96 |     return (JanetThread *)janet_unwrap_abstract(cfun(0, NULL));
      |                          ^
src/watchful.c: In function 'cfun_watch':
src/watchful.c:350:25: warning: implicit declaration of function 'janet_thread_receive' [-Wimplicit-function-declaration]
  350 |         int timed_out = janet_thread_receive(&out, timeout);
      |                         ^~~~~~~~~~~~~~~~~~~~

It might be a lot of work to rewrite this...

@pyrmont
Copy link
Owner

pyrmont commented Sep 1, 2021

Oh wow, do you actually use this? :P I never finished my blogging library so never got it to the point where I was relying on it :)

@subsetpark
Copy link
Author

I don't... yet :) But I am starting a new project that would benefit from it. Maybe the first step would be to extract the threaded parts from the basic bindings.

@pyrmont
Copy link
Owner

pyrmont commented Sep 1, 2021

Is the C interface for threads being deprecated? I compiled and ran this against the HEAD of master and while I get compilation warnings from Janet for my use of the thread/* functions in my Janet code, the C stuff all works fine.

If a rewrite is required, it looks to me like I'd probably want to use janet_ev_post_event to send the 'event' of the changed file to the event loop.

@pyrmont
Copy link
Owner

pyrmont commented Oct 4, 2021

@subsetpark OK, so I rewrote the whole thing as a cleanly separated C library and Janet wrapper. The Janet wrapper no longer uses the thread module and instead does everything on the event loop. Basic functionality appears to work but I don't have a comprehensive test suite at this point. I also haven't written documentation yet, unfortunately.

In case you wanted to play around with it, the idea now is that there are two ways to interact with the library, the easy way and the hard way:

  • Easy: watchful/watch takes a path to watch, a function to call on changes and some (optional) options. The call to watchful/watch returns a fiber. If the consumer wants to stop the watch cleanly, they call watchful/cancel on the fiber.

  • Hard: watchful/monitor will create a monitor object for a path with some (optional) options. The consumer can then use watchful/start and watchful/stop with the monitor object to start and stop the monitor manually. A call to watchful/start returns a channel. Events that are detected by the monitor are created as Janet structs and placed into the channel.

Although the Janet wrapper runs on the event loop, the C library still uses a separate thread to do the monitoring itself. When an event is detected by that monitoring thread, the thread calls a callback that was provided when the monitor was created (together with some user-specified data that was also provided when the monitor was created). The callback defined by the Janet wrapper serialises the pertinent information about the event into a pipe that the watchful module is monitoring on the main thread via the Janet event loop. When the main thread receives the data through the pipe, it transforms it into the Janet struct and puts it into the channel. As Rube Golberg-esque as that might sound, I think this approach is preferable to what I was doing before.

Oh, and it still only provides inotify and FSEvents backends. I would like at some point for there to be ReadDirectoryChangesW and kqueue backends but I want to be more confident the general approach is sound before attempting that.

@pyrmont pyrmont closed this as completed Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants