Poll shared cache connect #1489
Conversation
src/job_cache/job_cache.cpp
Outdated
| wcl::log::info("key = %s, sizeof(key) = %lu", key, sizeof(key))(); | ||
| memcpy(addr.sun_path + 1, key, sizeof(key)); | ||
| if (connect(socket_fd.get(), reinterpret_cast<const sockaddr *>(&addr), sizeof(key)) == -1) { | ||
| if (errno == EAGAIN) { | ||
| addr_bound = true; |
There was a problem hiding this comment.
The logic around the bool& here is tricky and hard to follow. You should just change the return type to wcl::result with an error enum of enum { Generic, AddrBound } which will make this function clearer but more importantly will make the callsite several times better
There was a problem hiding this comment.
Yeah that does seem better. I was thinking of doing something like that but I got lazy lol
|
I don't hate the overall structure and it does seem reasonable to me to limit daemon launches if we already know its launched. My vote would be to land if we can clean up the logic a little bit |
Yeah that makes sense, let me investigate how tokio and mio solve this first before going this route. If we do go this route then using a result to clean up the logic is preferable no doubt. |
|
I was thinking about this and my initial logic on this was wrong. I thought it would flip back and forth between launching and not launching but it actually should re-test for the connection being valid every time so it should keep not re-launching by continuously reconfirming that the other end of the socket is still bound. So this is actually quite a good strategy for reducing log churn, process churn, and should help reduce contention so I'm on board with this. I'll clean this up on Wednesday and we can land it. |
I'll start out by saying, I'm not a fan of this PR but I was just noodling on what we could do to help with this problem if it occurs in the future. Ideally we'd having a polling mechanism here but at this moment I'm not at all sure how to do that. It appears to me that Tokio and mio have solved this problem somehow so I think there's a better solution out there. This change does not eliminate the core issue like the previous PR did and because it at best reduces spurious daemon launches by 2x it doesn't eliminate DoS-ing yourself by spamming wake invocations, it just reduces the impact a little bit...I'm gonna put this up and see what people think but I'm rather on the fence about landing it.