-
Notifications
You must be signed in to change notification settings - Fork 178
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
endless poll loop in http_write when POLLHUP is returned #827
Comments
Hi @vliaskov , actually we have a RHEL customer experiencing issues with CUPS due the same reason - poll() gets POLLOUT|POLLHUP from kernel (in their case CUPS API function fails to connect to local cupsd after the first successful job) - I have a patch verified by the customer and I will send PR this week. |
@zdohnal If the POLLHUP is showing up in revents before all data has been read from the socket, IMHO that would be a bug or at least inconsistent with other Unix operating systems. Happy to review whatever fix you come up with... |
@vliaskov actually the revents are the same, but it happens in a different place for you - can you compile CUPS with debug-printfs, set cupsd to use debug printfs (by passing CUPS_DEBUG_LOG (location of log file), CUPS_DEBUG_FILTER (which messages to get - .*) and CUPS_DEBUG_LEVEL (0-9 - set it to 9) env variables to cupsd), enable debug logging via LogLevel in cupsd.conf and strace the cupsd when it happens? |
From the logs you showed it looks poll() returns nfds, so we shouldn't iterate in http_write with poll() callings... other polls can happen in send() and seeing debug_printfs can help. |
thank you for the input and suggestions @zdohnal . Sorry for the delay, I was on vacation. I will ask the customer for logs (they already had some but I think that the CUPS_DEBUG_FILTER .* will provide more info). Unfortunately the customer has a massive amount of queues and they claim that even CUPS_DEBUG_LEVEL=8 (debug) is not feasible for them without creating huge files, let alone CUPS_DEBUG_LEVEL=9 (debug2) . CUPS_DEBUG_LEVEL=7 (info) will be possible, not sure about the higher levels. I am also not sure what the file size limit is for github attachments. By the way, if there is a draft of the patch mentioned in #827 (comment) that can be used for testing, please point me to it. Although as you say the poll hangs happen at a different place in this case, so the patch is perhaps not covering this case... |
@vliaskov hmm... can you patch the debug_printfs in http_write() to be used in info level? I'm primary interested in those messages, so I would like to see their output - and then enable debug printfs for cupsd for CUPS_DEBUG_LEVEL=7? Additionally, just to be sure it would be great to add new debug_printfs at info level in http_write to get more idea about the execution.
Additionally, just to be sure in case if a TLS library is in play, add/change debug printf in _httpTLSWrite() CUPS implementation for your TLS library - it would be interesting to see the result and errno coming from TLS function such as After that, enable cupsd logging at debug2 (LogLevel debug2 in /etc/cups/cupsd.conf), restart it and attach strace ( It would be great to see all those files, because it will give us idea what's going on - IMO either glibc (send()) or TLS library (depends on what library you use to compile with CUPS in SUSE - it is GNUTLS in RHEL) does not take kindly to a descriptor with POLLHUP. Ad the patch - the core of the patch is to check SO_ERROR socket option on the descriptor (the same way as we do in cups/http-addrlist.c, httpAddrConnect2() for __sun), and if there is no error there, remove the POLLHUP from revents, which is later checked by the function. But we don't check revents in |
Thank you. Btw, do you mean to use We also use GNUTLS, there are already DEBUG_printf calls for the errno, do you want changes below?
|
DEBUG_printf(), because that's the one which is available for whole library. I found debug_printf() only in snmp backend. Ad gnutls - it would be great to see result and errno which come straight from gnutls call, so put the debug printf right after the command and print result and strerror(errno). |
understood, thanks. |
Customer is having issues with huge log files (and the issue takes at least a few hours to trigger). @zdohnal can you suggest a more restrictive CUPS_DEBUG_FILTER setting (not the capture-all setting ".*") that would still provide useful information to you for debugging, while avoiding capturing everything? Or is that not possible? |
IMO you can try filter it by 'http_write' - it should give us the more narrowed data. |
Thank you @zdohnal . Could the POLLOUT | POLLER this be caused by a half-closed TCP connection e.g. from a shutdown() on the client side? I am trying to think of ways to reproduce the issue, as I am currently unable to. What would be the proper way to handle the POLLHUP? If POLLHUP is detected while waiting for POLLOUT, we could simply clear the returned POLLHUP event, and proceed normally e.g.:.
I am not sure if the write should proceed though (or perhaps annulled with setting length=0), or if an error should be returned instead. |
Another possibility, using getsockopt() and SO_ERROR. If the socket contains an error, we return that.
|
@vliaskov see the last paragraph in #827 (comment) - your last comment is basically the same I described and did in our case. |
@zdohnal my idea (derived from the http-addrlist.c __sun fix) was:
The original suggestion:
If POLLHUP was returned but no SO_ERROR is found in socket: shouldn't that be a bug somewhere e.g. kernel? Or do you expect that POLLHUP would not be accompanied by with a socket error? If the disconnected socket does not have an SO_ERROR: can we return something appropriate anyway to break the loop e.g. ECONNRESET, EPIPE? It seems if we do not return any error, the poll loop will continue, as http_write will keep trying to write length amount of bytes to a closed socket.
And yes it's true that http_write does not currently check revents for POLLERR or POLLHUP, so I don't think setting or clearing them will make a difference. Also, why would we remove the POLLHUP from revents? Customer has issues testing at the moment, so no feedback yet... |
You forgot the patch I had mentioned is for a different issue with similar symptoms (as I mentioned previously, so that's why I asked for more data and didn't share the patch). I meant to remove POLLHUP only if it is accompanied by POLLIN or POLLOUT, as we see in strace - and tbh, I got the getsockopt() advice from our tech-list, so I expected SO_ERROR would reflect if there is really an error on socket, and if not, remove the event, because we check for it later. But that still about the similar issue we have CentOS Stream/RHEL, yours in different place - please get me at least part of strace logs with cups debug logs, so I can see where the daemon iterates when doing several polls. |
Thanks for the explanation, I understand now what you mean, since your patch is for a similar but different issue in httpAddrConnect2 . I will update once I have logs. |
Some Linux kernel versions put POLLOUT|POLLHUP into revents when client tries to connect with httpAddrConnect2(), which makes the connection fail. Let's check the option SO_ERROR before scratching the attempt - if there is no error, remove POLLHUP from revents. I've re-purposed previously Solaris-only code to be used everywhere if the conditions are met - this should prevent bigger delays than necessary. Slightly different issue than OpenPrinting#827, but with similar symptoms (kernel sending POLLOUT|POLLHUP).
Some Linux kernel versions put POLLOUT|POLLHUP into revents when client tries to connect with httpAddrConnect2(), which makes the connection fail. Let's check the option SO_ERROR before scratching the attempt - if there is no error, remove POLLHUP from revents. I've re-purposed previously Solaris-only code to be used everywhere if the conditions are met - this should prevent bigger delays than necessary. Slightly different issue than OpenPrinting#827, but with similar symptoms (kernel sending POLLOUT|POLLHUP).
Some Linux kernel versions put POLLOUT|POLLHUP into revents when client tries to connect with httpAddrConnect2(), which makes the connection fail. Let's check the option SO_ERROR before scratching the attempt - if there is no error, remove POLLHUP from revents. I've re-purposed previously Solaris-only code to be used everywhere if the conditions are met - this should prevent bigger delays than necessary. Slightly different issue than #827, but with similar symptoms (kernel sending POLLOUT|POLLHUP).
cups-7427-loglevel9.log.zip @zdohnal I don't have customer traces/logs yet. However, I can trigger
strace, debugging log attached. Debugging patch used is pasted further below. I see the POLLOUT|POLLHUP triggered for a lot of small buffer http_writes. There is no EINTR, EAGAIN or nfds < 0 logged in the poll while loop, as far as I see. Most of the errors result in EPIPE (either in getsockopt, or copied later in errno): When POLLOUT|POLLHUP revents are returned, almost all buffers get handled in 2 iterations of the http_write poll loop:
However, the same fd keeps getting polled with new http_write requests, usually for 4-5 byte buffers. e.g.:
Note: timestamps are off by an hour in strace versus debugging logs, but they match. This probably does not replicate the exact customer issue : the trace excerpt in original issue description only shows polling calls in an endless loop, but in these logs the loop breaks. But let me know if it gives any insights or ideas. I can also retake logs, or add more debugging output if needed. I can also increase GNUTLS_DEBUG_LEVEL. gnuTLS used is 3.7.3 (without kernel offload, i.e. no kTLS) Debugging patch used for the current logs also pasted below:
Another possibility I thought of was that the customer is using kTLS and getting stuck in a sendmsg() loop due to this gnutls patch not applied and keep returning EAGAIN/EINTR: https://gitlab.com/redhat/centos-stream/rpms/gnutls/-/commit/e99bcaff789941a94416996daeb423df295d5443 |
Some Linux kernel versions put POLLOUT|POLLHUP into revents when client tries to connect with httpAddrConnect2(), which makes the connection fail. Let's check the option SO_ERROR before scratching the attempt - if there is no error, remove POLLHUP from revents. I've re-purposed previously Solaris-only code to be used everywhere if the conditions are met - this should prevent bigger delays than necessary. Slightly different issue than #827, but with similar symptoms (kernel sending POLLOUT|POLLHUP).
I forgot to say the results/traces of my last commit are with Producing the POLLOUT|POLLHUP revents in
and issuing |
Hi @vliaskov , do you have data from customer? I see you were able to get POLLOUT|POLLHUP from poll(), but are you certain it is the action which happens for customer? I've checked your data - it really seems you missed
right after the poll() - I see only:
which is called after getsockopt() - it is good to know, but we have to know what errno is at least after poll(), and ideally even before poll() - we have to find out where errno is set, because GNUTLS sending function fails due invalid session. ====================================== But first and foremost please find whether your reproducer is what your customer experience - otherwise we will spend time on getting fix which won't help your customer in the end. |
@vliaskov So if you are seeing this in groups of 3, this is expected since the chunk writing does everything it can to avoid blocking indefinitely. After the poll call is a send/write which will return an error if the other side is really shut down, so I'm not sure why POLLHUP is set in this case... Ultimately we need to figure out how to reproduce this, otherwise we are completely in the dark on a solution... |
Closing for now; if you find a way to reproduce, please let us know and we'll re-open... |
Describe the bug
Customer observes that cupsd occasionally gets stuck in a poll() loop. strace shows:
If POLLHUP is received (i.e. the connection's other end was closed?), should we continue polling? Should POLLHUP be checked in the revents to break the loop? I haven't yet found an obvious upstream fix for this, so I am opening the bug.
Backtrace of the cupsd process:
To Reproduce
Unfortunately, I am still not clear on how to reproduce this. Probably clients need to be closed while cupsd is sending data to them. I will update if I have an exact reproducer from the customer.
If there are ideas on how to stress test this let me know. Is there an existing testcase that would stress this scenario?
Expected behavior
Cups should not keep polling indefinitely if the other side has hung up.
System Information:
The text was updated successfully, but these errors were encountered: