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

Compatibility with fiber scheduler. #92

Closed
ioquatix opened this issue Feb 14, 2023 · 7 comments
Closed

Compatibility with fiber scheduler. #92

ioquatix opened this issue Feb 14, 2023 · 7 comments

Comments

@ioquatix
Copy link

ioquatix commented Feb 14, 2023

Hello, thanks for this awesome work.

Is this adapter compatible with the fiber scheduler?

Thanks.

@ioquatix
Copy link
Author

ioquatix commented Feb 14, 2023

Hmm I found https://github.com/github/trilogy/blob/2dd91e5d1974b29473ad646e5dba0a50562fe37a/contrib/ruby/ext/trilogy-ruby/cext.c#L214

Would be more efficient if we could use rb_io_wait but this is a good start.

@ioquatix
Copy link
Author

I'm not sure where this is used, but it might be better to use the rb_wait_* functions too:

https://github.com/github/trilogy/blob/2dd91e5d1974b29473ad646e5dba0a50562fe37a/src/socket.c#L50

@composerinteralia
Copy link
Contributor

I'm not sure where this is used, but it might be better to use the rb_wait_* functions too:

https://github.com/github/trilogy/blob/2dd91e5d1974b29473ad646e5dba0a50562fe37a/src/socket.c#L50

_cb_wait is not used at all in the Ruby gem. That's the default for anyone using the C lib directly, but for Ruby we swap it out at https://github.com/github/trilogy/blob/2dd91e5d1974b29473ad646e5dba0a50562fe37a/contrib/ruby/ext/trilogy-ruby/cext.c#L251-L253 with a wait_cb using rb_wait_for_single_fd.

That's about the extent of my knowledge at the moment. I'd happy to help make whatever changes are desirable here, but I'll probably need to learn a lot more about the fiber scheduler, the difference between rb_wait_for_single_fd and rb_io_wait, etc.

@ioquatix
Copy link
Author

I am happy to make a PR if you are willing to work with me getting it merged?

@composerinteralia
Copy link
Contributor

I am happy to make a PR if you are willing to work with me getting it merged?

Yes, I can do that!

@ioquatix
Copy link
Author

ioquatix commented Feb 24, 2023

I started playing around with the code.

static int _cb_ruby_wait(trilogy_sock_t *sock, trilogy_wait_t wait)
{
    struct timeval *timeout = NULL;
    int wait_flag = 0;

    switch (wait) {
    case TRILOGY_WAIT_READ:
        timeout = &sock->opts.read_timeout;
        wait_flag = RB_WAITFD_IN;
        break;

    case TRILOGY_WAIT_WRITE:
        timeout = &sock->opts.write_timeout;
        wait_flag = RB_WAITFD_OUT;
        break;

    case TRILOGY_WAIT_HANDSHAKE:
        timeout = &sock->opts.connect_timeout;
        wait_flag = RB_WAITFD_IN;
        break;

    default:
        return TRILOGY_ERR;
    }

    if (timeout->tv_sec == 0 && timeout->tv_usec == 0) {
        timeout = NULL;
    }

    int fd = trilogy_sock_fd(sock);
    if (rb_wait_for_single_fd(fd, wait_flag, timeout) <= 0)
        return TRILOGY_SYSERR;

    return 0;
}

The rb_wait_for_single_fd should be replaced with rb_io_wait(io, RB_INT2NUM(wait_flag), rb_fiber_scheduler_make_timeout(timeout)).

The io object can be created by using RB_IO_OPEN but I'll need to check the exact syntax.

I think the callback argument will need to be added or the existing context should be changed to store the io, e.g.

struct trilogy_ctx {
    trilogy_conn_t conn;
    VALUE io;

    char server_version[TRILOGY_SERVER_VERSION_SIZE + 1];
    unsigned int query_flags;
};

Do you have any thoughts about how to best achieve this? Maybe we can add a void *context to struct trilogy_sock_t which refers to trilogy_ctx.

Also, just for your reference, naming conventions with trailing _t is highly discouraged by POSIX standard. https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html#tag_22_02_12 - _t suffix is reserved for system types.

@ioquatix
Copy link
Author

I've opened a draft PR with the above changes.

@composerinteralia composerinteralia transferred this issue from trilogy-libraries/activerecord-trilogy-adapter Jun 19, 2023
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