Exiting the loop previously could cause watcher to busy wait (i.e.
rebuild the array and call poll() repeatedly) until the active callback
was done.
Assume watcher observes two FDs 15 and 22, which are in the list in that
order. FD 15 is signaled and its callback gets triggered. The array of
FDs is rebuilt and does not include 15 anymore. Now FD 22 is ready for
reading. However, when enumerating all registered FDs, the loop previously
was exited when reaching FD 15 and seeing that it's active. FD 22 was
never checked and the array was immediately rebuilt and poll() called.
If the callback for 15 took longer, this was repeated over and over.
This basically reverts d16d5a245f ("watcher: Avoid queueing multiple
watcher callbacks at the same time"), whose goal is quite unclear to me.
If it really wanted to allow only a single callback for all FDs, it didn't
achieve that as any FD before an active one would get notified and if
multiple FDs are ready concurrently, they'd all get triggered too.
Skipping entries with active callback makes sense as it avoids a lookup
in the FD array and subsequent revents checks. But why would we need to
rebuild the array if we see such an entry? Once the callback is done,
the watcher is notified and the array rebuilt anyway (also if any other
FD was ready and jobs get queued).
The list of FDs is recreated quite often (e.g. due to the kernel-netlink
event sockets) and if a logger depends on watcher_t in some way this
might cause conflicts if the mutex is held.
Since the same FD may be added multiple times (e.g. with separate
callbacks for WATCHER_READ and WATCHER_WRITE), the previous check
might not have found the correct entry. As the entry can't be removed
while in a callback, the pointer comparison is fine.
This could be problematic in case loggers in some way rely on watcher_t
themselves. This particular log message should rarely occur if at all,
but still avoid holding the mutex.
This can happen if a stream is used blocking exclusively (the FD is
never registered with watcher, but is removed in the stream's destructor
just in case it ever was - doing this conditionally would require an
additional flag in streams). There may be no thread reading from
the read end of the notify pipe (e.g. in starter), causing the write
to the notify pipe to block after it's full. Anyway, doing a relatively
expensive FD update is unnecessary if there were no changes.
Fixes#1453.
This avoids the evaluation of %N even if the thread pool is never used.
We need to avoid as many custom printf specifiers as possible when
fuzzing our code to avoid excessive log messages.
With LinuxThreads, poll() is unfortunately no cancellation point. It seems
that poll gets woken up after cancellation, but we actively must check
for cancellation before re-entering poll to properly shut down the watcher
thread.
While they usually are not included in a normal strongSwan build, the XPC
header indirectly defines these Mach types. To build charon-xpc, which uses
both XPC and strongSwan includes, we have to redefine these types.
poll() may return POLLHUP or POLLNVAL for given file descriptors. To handle
these properly, we signal them to the EXCEPT watcher state, if registered. If
not, we call the read/write callbacks, so they can properly fail when trying
to read from or write to the file descriptor.
If file descriptors get added and removed in rapid succession, the active
watcher thread might not take notice of it and continues running. However, add()
spawns a watcher thread whenever a file descriptor is added to an empty set.
This could result in multiple watcher threads, which is fixed by a proper
check for running watchers.
Instead of a pipe we use a TCP socketpair (can't select() a _pipe()), and
Windsock2 send/recv functions instead of read/write.
Currently supported (and required) are file descriptors provided by Winsock
only; we might use a separate mechanism for traditional file handles if
required (or switch to Windows events and WaitForMultipleObjects) for a future
version.
During shutdown, waiting for callbacks might never complete, as queued
callbacks might not get executed under certain conditions. Not the clean fix,
but works good enough for now.
Seen on Windows in vici tests.
While we don't add FDs with an active callback to the watched FDSET, we still
can get notifications for callbacks active due the asynchronous processing
of the same.
To avoid queue multiple callbacks, we check for queued callbacks before
activating new ones.
This should make sure we refresh the fdset if a user closes an FD it just
removed. Some selects() seem to complain about the bad FD before signaling the
notification pipe.
During daemon shutdown, some idle threads might be lingering around even if
set_threads(0) already has been called. To avoid any races, we enforce
synchronous execution of the job.
If all worker threads are busy and waiting for an event, we must ensure that
a job delivering that event gets executed. This new method has this property
for CRITICAL jobs, using a worker if we have one, but executing the job directly
if not.
During daemon shutdown, users might call remove() after processor.set_threads(0)
has been called. This gets problematic, as a watch event might be unable
to signal completion when no threads are available anymore. Work around this
issue by cancelling waiters once processor.cancel() has been called.
If a lock is held when queue_job() is called and the same lock is
required during the destruction of a job, holding the internal lock
in the processor while calling destroy() could result in a deadlock.
Tobias Brunner