Ruby: Implement Client#discard! to cleanly dispose of connection post fork #65
Conversation
…fork If you use `Client#close` on a connection that was inherited from a parent it will close the connection for the parent. Instead we need to use some POSIX magic to close the file descriptor without emitting any close notification, so that the server isn't mislead into closing the connection on its side.
casperisfine
changed the title
contrib/ruby/ext/trilogy-ruby/cext.c
Outdated
| int sock_fd = trilogy_sock_fd(ctx->conn.socket); | ||
| if (sock_fd >= 0) { | ||
| int null_fd = open("/dev/null", O_RDWR | O_CLOEXEC); | ||
| if (null_fd < 0) { | ||
| trilogy_syserr_fail_str(errno, rb_str_new_cstr("Failed to open /dev/null")); | ||
| return Qfalse; | ||
| } | ||
|
|
||
| if (dup2(null_fd, sock_fd) < 0) { | ||
| trilogy_syserr_fail_str(errno, rb_str_new_cstr("dup2 failed")); | ||
| } | ||
| close(null_fd); | ||
| } |
There was a problem hiding this comment.
I don't know if it's better to have all this logic here, or it it would make sense to implement a trilogy_discard in the C library that could be used by other potential bindings?
There was a problem hiding this comment.
IMHO it would make sense to add a method to the C API that handles duping the /dev/null file descriptor to the socket fd, and returns either TRILOGY_OK or TRILOGY_SYSERR, and then the cext could handle raising the ruby error.
contrib/ruby/ext/trilogy-ruby/cext.c
Outdated
| int null_fd = open("/dev/null", O_RDWR | O_CLOEXEC); | ||
| if (null_fd < 0) { | ||
| trilogy_syserr_fail_str(errno, rb_str_new_cstr("Failed to open /dev/null")); | ||
| return Qfalse; |
There was a problem hiding this comment.
Won't raising in the prior line prevent us from returning false here?
There was a problem hiding this comment.
Yeah, it's unreachable, but help making clear we stop here.
contrib/ruby/ext/trilogy-ruby/cext.c
Outdated
| int sock_fd = trilogy_sock_fd(ctx->conn.socket); | ||
| if (sock_fd >= 0) { | ||
| int null_fd = open("/dev/null", O_RDWR | O_CLOEXEC); | ||
| if (null_fd < 0) { | ||
| trilogy_syserr_fail_str(errno, rb_str_new_cstr("Failed to open /dev/null")); | ||
| return Qfalse; | ||
| } | ||
|
|
||
| if (dup2(null_fd, sock_fd) < 0) { | ||
| trilogy_syserr_fail_str(errno, rb_str_new_cstr("dup2 failed")); | ||
| } | ||
| close(null_fd); | ||
| } |
There was a problem hiding this comment.
IMHO it would make sense to add a method to the C API that handles duping the /dev/null file descriptor to the socket fd, and returns either TRILOGY_OK or TRILOGY_SYSERR, and then the cext could handle raising the ruby error.
|
@adrianna-chang-shopify done. There's now a |
There was a problem hiding this comment.
Thanks @casperisfine! Just one question, afterwards I'll merge.
| struct trilogy_ctx *ctx = get_ctx(self); | ||
|
|
||
| if (ctx->conn.socket == NULL) { | ||
| return Qtrue; |
There was a problem hiding this comment.
Curious about returning true here instead of nil -- can you explain your reasoning?
There was a problem hiding this comment.
Not a lot of reasoning honestly, I generally return true on success, but it depends a lot on context. But the return value doesn't matter much here TBH.
There was a problem hiding this comment.
But the return value doesn't matter much here TBH.
Yeah that's fair. I'm not sure I'd define calling discard! on an already-closed connection "successful", but doesn't matter a whole lot as you've said.
If you use
Client#closeon a connection that was inherited from a parent it will close the connection for the parent.Instead we need to use some POSIX magic to close the file descriptor without emitting any close notification, so that the server isn't mislead into closing the connection on its side.
cc @matthewd as we just discussed this.
and cc @adrianna-chang-shopify as the trilogy-adapter will need to be updated to use that new method: https://github.com/github/activerecord-trilogy-adapter/blob/592d98f25ab3e21e0d4f0d9ebc51bb932fd547b3/lib/active_record/connection_adapters/trilogy_adapter.rb#L160