thread-unsafe call of reader_interrupted() in wildcard_expand_internal()

[JanKanis]

This bug is a spinoff from bug #516.
wildcard_expand_internal is called both on the main thread when executing a command with wildcards, and on background threads when searching for autocompletions. reader_interrupted should not be called on background threads.

[JanKanis]

Question: is there currently any way in which an autocompletion thread can be notified that its service is no longer needed? That would probably be desirable if e.g. a slow wildcard is being searched for for an autocompletion and then the user types another key which makes the autocompletion irrelevant.

[ridiculousfish]

We do have a generation count in reader.cpp, but it's used only for an early exit when an autosuggestion thread is started. It's not used once the thread is already running.

That would be a nice thing to add. It would not necessarily be too hard. You would propagate the the gen count at the time the thread started throughout the call to expand(), and compare it to s_generation_count to detect cancellation.

[JanKanis]

I did an implementation in branch bug-537, using thread-local storage.

Problems: thread local storage with the __thread keyword is supported in gcc but afaik not in clang and therefore I guess not in XCode. @ridiculousfish could you check if by any chance I'm wrong about that?

Problem 2: I copied a configure check for TLS from a gnu repository, but it's GPLv3+, so I think it's incompatible with fish. Maybe there's some loophole as output of auto(re)conf is exempted.

Is there any way to make threadlocal storage work on platorms we care about?

[ridiculousfish]

AFAIK, thread local storage is not supported at all on OS X. This is because it requires changes to the executable format (thread-local sections), as well as the compiler and linker. I do not know the TLS status on *BSD, but I am not optimistic.

OS X has pthread_setspecific, which is part of POSIX and so has a better chance of being widely supported. There's also a possibility of storing a value in struct WorkerThread_t in iothread.cpp - we could roll our own thread local storage using pthread_self in that structure. This is appealing because it would give us more control - we wouldn't have to worry about the vagaries of TLS on different platforms.

Yet another possibility is to use pthread_kill to inform threads when they should die. This is appealing, because it would allow us to unify it with normal signal handling. Wildcard expansion would cancel either if it's on the main thread and the user hit control-C, or it's on a background thread and received a cancellation signal. However, this would require a mechanism to record that a thread received a signal.

Also, is it really intractable to pass some data explicitly, all the way down? That would be cleanest.

[JanKanis]

Using pthread_kill would (as you note) require a mechanism to record that a signal was received, i.o.w. a thread-local location to store this status. So that doesn't buy us anything. Threading the thread-local context through all the calls a thread makes doesn't sound very attractive to me. In this case there are six stackframes between the first call to wildcard_expand_internal and threaded_autosuggest, which will need to pass information unrelated to what they are doing themselves, and if such a mechanism were implemented for history searches or other things all calls related to those would also have to pass the extra context.

I opted for tls in this branch because it was easiest to do as a first approach. Tls also seems to me to be the "right thing" in this case, if it weren't for the portability difficulties. So my preference would be to use pthread_setspecific or roll our own with the WorkerThread_t.

[JanKanis]

I've implemented a version using pthread_setspecific. Mainly because that was next-easiest. Is it really a problem to depend on this? We're already using pthreads and it is part of the pthreads specification. It has a fast (couple of instructions, apart from the function call) implementation on both linux (ubuntu) and OSX.

by the way, I was planning on merging bug-read-ctrlC and then bug-537 when the code is good. Is that ok or should I squash or rebase?

[ridiculousfish]

pthread_setspecific is OK to rely on.

Changes in both those branches look great! I'll leave it to your discretion how you merge. My only suggestion is to rename reader_cancel_thread so it doesn't imply that it triggers a cancellation, e.g. reader_is_thread_cancelled or something else.

Thanks!

[JanKanis]

fixed with the merge in 9a89da3. (d9d8bfa should be ignored)