Revisit EINTR handling with check_interrupts = TRUE

[krlmlr]

• Linux man on select states that the EINTR should be interpreted as "your select() call has been interrupted, try again". Which is in contrary to your current implementation.

From #337 (comment).

@zozlak: I don't follow where the man page you pointed to suggests that it's mandatory to call select() again. Can you please clarify?

[zozlak]

Last sentence o the third paragraph of the notes section states (citing whole paragraph):

On some other UNIX systems, select() can fail with the error EAGAIN if the system fails to allocate kernel-internal resources, rather than ENOMEM as Linux does. POSIX specifies this error for poll(2), but not for select(). Portable programs may wish to check for EAGAIN and loop, just as with EINTR.

[krlmlr]

Yes, that's what I see too. To me, it seems that the text focuses on EAGAIN and only mentions retrying on EINTR as a possibility (if you want to ignore interrupts), not as mandatory. What am I missing?

[zozlak]

The thing is (in contrary to the Ms Windows documentation) there is no list of possible signals which can interrupt the select() and the list of standard signals (section "Standard signals") is long and includes a lot of stuff which we just don't care about while waiting for the response from the Postgresql server. Let's say someone sent a USR1 signal and it caused select() to return with the EINTR (I don't know if it's possible in practice and I'm too lazy to go into the kernel code of all *nixes R can run on to check it but the documentation I'm referring to warns us that in general everything is possible and a portable code should take it into account). Should we abandon SQL query execution because of that? I would say no and we should just repeat the select().

[krlmlr]

Thanks for the clarification. IIUC select() will return on any signal, not only SIGINT. How can we distinguish what signal has been sent after select() has returned? Should we establish a signal handler? Unfortunately, Rcpp::checkUserInterrupt() doesn't cut it, at least on my system.

It seems that the tests capture the desired behavior. Would you like to help with a fix?

[zozlak]

First, we only care about whatever R considers a user request for execution interruption. This may or may not correspond to the SIGINT signal. IMHO we don't want to think about it. We definitely want some abstraction level here and as far as I can tell that's what Rcpp::checkUserInterrupt() provides.

Second, the documentation says only that the select() may be interrupted by any signal. It doesn't mean it does is interrupted by any particular signal on any particular platform and the lesson learned from the problems we encountered with completely different select() behavior on Windows is that the "if it works like that on my machine, it must work like that everywhere" approach is a bad approach :-) . Therefore we shouldn't make the assumption that a particular signal like SIGINT interrupts the select(). Anyway, IMHO we shouldn't think about it in terms of OS signals in the first place (see the previous paragraph).

Summing up on *nix systems I would treat select() returning the EINTR in the same way as select() returning 0 - just call the try{Rcpp::checkUserInterrupt();}catch(...){return false;} and call the select() again. If the select() has been interrupted by an event which will be recognized as the request to terminate the execution by the Rcpp::checkUserInterrupt(), it will terminate and if it was any other event we don't care about here, we will continue waiting.

[krlmlr]

Thanks. Happy to review an implementation. Do you think our existing tests capture the desired behavior?

Rcpp::checkUserInterrupt() did not allow me to detect that a user wanted to interrupt. I'm not familiar with its internals. Do we need a signal handler for the duration of select()?

[krlmlr]

To be more precise: the implementation is opt-in, and works (according the tests in test-checkInterrupts.R and test-immediate.R) on the major platforms. There's an inconsistency I couldn't fix in the "immediate with interrupts after notice" test on Windows, that's all. If we can trust the tests (and please tell me if they are bad), this means that we can achieve the desired result most of the time.

On the other hand, the argument regarding EINTR in select() and what we should or should not do sound like pure theory without immediate practical relevance. Can you show me where our implementation doesn't do what it should be doing, in a reproducible example?

I hear you -- ideally we could trust Rcpp::checkUserInterrupt() and not worry about signals, but it doesn't look like we can.

[zozlak]

I need a little time to look at the tests and perform a few experiments by myself. I hope I will do it by the end of the week.

When you say the Rcpp::checkUserInterrupt() doesn't work, do you mean the "check_interrupts = TRUE interrupts immediately (#336)" test fails with an implementation depending solely on the Rcpp::checkUserInterrupt()?

Also, what's the inconsistency between *nix and Windows you couldn't fix in the "check_interrupts = TRUE interrupts immediately (#336)" test?

[krlmlr]

Ctrl+C (SIGINT) interrupts select(), it returns EINTR, but then Rcpp::checkUserInterrupt() doesn't detect it.

It was another test I couldn't fix on Windows -- I vaguely remember it was related to slightly different but still correct-ish output. If you have access to Windows, I'm curious if it's fixable. Search for skip_on_os("windows") .

[krlmlr]

Thanks for your help!

[zozlak]

First, sorry for the delay. I overestimated amount of my free time.
Getting back to the topic I did some testing and it looks like nothing really interrupts signal(), at least under my linux, but the SIGINT is trapped and handled somewhere else.

As a preparation I added very verbose debugging messages allowing me to track the whole execution track step by step. I also took a look how things look like on the Postgresql side. It turned out that on my platform (ubuntu 21.10, kernel 5.13.0):

• When select() returned, its return code was always 0. Moreover Rcpp::checkUserInterrupt() never detected user interrupt (never thrown an exception).
• When the SIGINT was sent to the rsession process, the R session stopped execution of the current code but it was done outside of the PqResultImpl::wait_for_data() and the execution of the PqResultImpl::wait_for_data() was never resumed:
  • I never got the debug message set up for the select() return code being lower than 0.
  • I also never go the debug message set up for the select() return code being 0.
  • I never got the debug message set up after the do {} while (PQisBusy(pConn_)); loop.
  • Looking on the postgresql side the execution of the SQL query has never been terminated.
• select() wasn't interrupted by other non-terminating-process-execution-signals as well, no matter if they affected the R session process (e.g. SIGSTOP/SIGTSTP followed by SIGCONT) or not (e.g. SIGUSR1, SIGUSR2).

All in all it looks like:

• In practice under linux it doesn't really matter how we handle EINTR reported by the select() as we are unable to trigger such a scenario. Anyway according to the linux man page on signals (section "Interruption of system calls and library functions by signal handlers", bullet point mentioning select()) the most kosher implementation seems to be to threat in the same way as timeout/new data and just continue the waiting loop.
• The code execution interruption triggered by the SIGINT is handled somewhere else. Most probably by the SIGINT handler registered by the R internals. On one hand it's good because it's handled without any action needed from our side and it looks like we don't even need to call the Rcpp::checkUserInterrupt() periodically. On the other hand it's bad because it brutally terminates execution of our library code leaving us with no chances to run any cleanup code (especially aborting the query execution). It's a good question if/how this problem can be addresses but it goes far beyond my knowledge. We should probably ask someone familiar with R internals about it.

[krlmlr]

Thanks.

• Can you please share the code with the added debugging messages?
• What do things look like on Ubuntu 20.04 and on other systems? I could test 20.04.
• Do you think the automated tests are testing the right thing? Do they succeed on your system?

It worries me that the query is not terminated on the server side. Let's get to the root of this.

[zozlak]

• https://github.com/zozlak/RPostgres/blob/issue-344/src/PqResultImpl.cpp#L592-L634 Sorry for "debugging by printing messages" but I don't develop in C/C++ both in general and specifically in R so I'm not aware of existing mechanisms (and I'm too lazy to learn them). Then I'm testing it by hand with

  library(RPostgres)
  c = postgresDefault(check_interrupts = TRUE)
  dbGetQuery(c, "SELECT pg_sleep(5)")

  and sending signals from console, e.g.

  kill -s SIGINT `ps ax | grep rsession | grep -v grep | sed -e 's/?.*//'`

  In my case uninterrupted execution gives

  entering the loop
  timeout 
  no checkUserInterrupt after timeout
  timeout 
  no checkUserInterrupt after timeout
  timeout 
  no checkUserInterrupt after timeout
  timeout 
  no checkUserInterrupt after timeout
  PQisBusy returned false
  entering the loop
  timeout 
  no checkUserInterrupt after timeout
  PQisBusy returned false
  

  while interrupted with the SIGINT

  entering the loop
  timeout 
  no checkUserInterrupt after timeout
  timeout 
  no checkUserInterrupt after timeout
  timeout 
  no checkUserInterrupt after timeout
  

  As you can see there is no SOCKET_EINTR, PQisBusy returned false nor checkUserInterrupt after timeout message and one of the should pop up if the return statement inside wait_for_data() loop is reached or the loop ended.
• I don't know about other systems but trapping the SIGINT and killing RPostgres execution rather comes from R so I don't expect different behavior, at least on other linuxes.
• test-checkInterrupts.R looks fine to me, just this test should rather call pg_sleep() with value greater than 1 second if we want to test a query taking longer than 1 second :-)

[krlmlr]

Need to test this in 20.04. If the 21.10 behavior is different, we might need to take action.

[krlmlr]

Also, good catch regarding the mismatch between test description and feature tested.