It seems that when TCP_NODELAY fails to be set,
the failure is permanent. As such, there is no
point in filling the logs with the same error.
This patch logs the error only once, per process,
and supresses further logs from Socket::setNoDelay().
Change-Id: I52c6b8cca35a8c281b4c4639d61a7e2521775d49
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Instead of fighting the SolarMutex to get the messages
processed by the main loop. Simple and no additional
threading, mutexes etc.
messages from the external uno client are just
written to URPtoLoFD and the core reads from that
messages to the external uno client are written
to URPfromLoFD by core, that fd is in poll, and
activity there triggers a read by the DocBroker
to send it to the external uno client.
Signed-off-by: Caolán McNamara <caolan.mcnamara@collabora.com>
Change-Id: Ib1f0a0d5fb5ab22eee476d5d740b290c51de59dc
Failed with:
fuzzer/HttpEcho.cpp:111:17: error: 'removeSockets' is a private member of 'SocketPoll'
And:
kit/Delta.hpp:208:(.text._ZN14DeltaGenerator14DeltaBitmapRow7initRowEPKjj[_ZN14DeltaGenerator14DeltaBitmapRow7initRowEPKjj]+0x127): undefined reference to `simd_initPixRowSimd'
Signed-off-by: Miklos Vajna <vmiklos@collabora.com>
Change-Id: I6d980698b43ca3545d9eae5f40eabaf4e442ca17
map is never null so make it a reference
Signed-off-by: Caolán McNamara <caolan.mcnamara@collabora.com>
Change-Id: I9f6ad863d0a1102dd7e26a5668b1c6ba71249580
Remove un-necessary gating of wakeup() on stop un-related to
callbacks or new sockets.
This reverts commit 25f2581a30.
Change-Id: I9f9f30fed34c973b86206677168071abb81afa6f
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
This merges multiple implementations of
assertCorrectThread and simplifies its
usage.
Change-Id: I7be5dea62c6046fb0412d7f885fcbcc4b66e3fd9
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
A rare edge-case happens when we have a socket
that is closed (and we get POLLHUP or POLLERR)
but upon reading the socket we get EAGAIN.
This was observed when simulating EAGAIN,
and it is possible that this is quite impossible
in practice (since we read only when we get
POLLIN), but at least for the unit-tests
we need to handle this case, so we don't
get random failures.
Change-Id: I77af1726066507af5d5ada68fe11b479a4e579e5
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
There is no use-case for calling the
thread function of the SocketPoll
from outside.
Change-Id: Id8e87369494817aaab749d03d1cd4cd3724c2da1
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
When shutting down, SocketPoll threads asynchronously
stop and exit. This fundamental race means the warning
is useless and noisy.
Change-Id: I3ca9044c9a68689abb7e8f692fffd10509eadab6
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
This avoids wakeup warning us that the thread is not started to wakeup.
Change-Id: Ifd5c483717024cb1c30521433d0d10acead01e2f
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
A corner case where read doesn't return 0
on a closed socket can result in a stuck
state where we attempt to read when we
do get ECONNRESET (which we didn't check
on reads).
This makes the interface of readIncomingData
the same as writeOutgoingData by returning
the actual return value of the read syscall.
So now we handle both return 0 as well as
error codes returned on failed calls.
The logic hasn't changed, just that now
we handle errors better and similar to the
write case.
Change-Id: I0b38a63da4e6c92a482948478d5d8d446e0b8b58
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
This enables code that was protected with
EnableExperimental in the socket logic (and one
case in DocBroker). These changes are now deemed
safe to enable permanently.
Change-Id: Ie62f5d7bd281ade90f38d654b51b104b8d1f14bc
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Without knowing whether the write succeeded
or failed, we cannot trust errno has our
error or some earlier and unrelated error.
This was caught when there were two sockets,
one of which disconnected. The write to the
disconnected one returned -1 and set errno
to ECONNRESET. We subsequently wrote to the
second socket, which succeeded. However,
because errno wasn't reset, and since
writeOutgoingData didn't return anything
to indicate the success, errno's ECONNRESET
value meant the second socket was also
disconnected, which was incorrect.
writeOutgoingData now returns the last return
value from writeData so we can make informed
decision as to whether to check errno or not.
Also, to avoid incorrecly propagating errno,
we now capture errno only when readData and
writeData return -1. This has the nice
side-effect that we reset errno to 0 when
no errors occur during our call.
Change-Id: I911b31390f37cc71938bc4a6ae75393dbf24bb9d
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Since both implementations are identical, there
really is no benefit to having two version.
Change-Id: I4a5288243291c0d5706df8e8870b918fab425317
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
The last burst of data arrives with POLLHUP.
If during the read we hit any errors, including
EAGAIN, for whatever reason, we may lose the
data if we don't attempt reading again later.
The only way to guarantee that we do not have
any more data to read, when we get POLLHUP,
is to poll until we either get no POLLIN or
read returns 0 indicating the socket is closed,
Oddly, read(2) sometimes returns 0 without
POLLHUP ever being set, and sometimes we
do get POLLHUP while POLLIN still set. This
implies that poll and read aren't consistent
in how they detect and report the close
condition. Luckily, with this patch we can
handle all cases without any complications.
This was caught through simulateSocketError,
which when it injects EAGAIN on the very
last read, when the socket is closed, causes
error as the response is partially or
completely lost (because we never read it).
The behavioral change should only be
observable with EnableExperimental.
Change-Id: I77260f98d5dd5050c5f9b202b9effd501191336b
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
This extends the detection of connection termination
for writes via ECONNRESET and EPIPE.
For some reason ECONNRESET was not detected.
This patch avoids extra polling to detect connection
termination and makes the cleanup faster.
The changes are guarded by EnableExperimental to avoid
any unexpected change of behavior.
Change-Id: I2ae3803f025d3cf756f86460e47aedc984249509
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Since the hostname argument is passed
to both the base class of SslStreamSocket
and SSL_set_tlsext_host_name, and since
the base class's getter, also called
hostname(), is hidden by the argument,
we cannot move it.
An empty hostname can result in 403 Forbidden
from the server due to missing Server Name
Indication (SNI).
Change-Id: I27990f64f17ec3c81a4dd543a078807629cd0c20
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
And improved socket logging in general while
making them more consistent.
Change-Id: I1ed7f2561476ca5370af91079d5d616804396f8e
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
While SSL is handshaking, there can be no general
application data communication. During that early
stage of connecting we have data to send (the
request, headers, etc.) and so we poll on POLLOUT.
Naturally, we also always want to poll on POLLIN,
because we can never know when there is data to
read (especially true for web-sockets).
The problem is when SSL will not send data just
yet because it is handshaking. It is typically
waiting for handshake negotiation data to read,
so when we POLLOUT, poll immediately returns, but
writing (via SSL_write) fails with WANTS_READ
error. This goes on in a busy-loop until the
negotiation data is available for read and the
handshake is completed. Very inefficient.
The solution is to poll on whatever SSL needs
during the handshake, exclusively. Once the
handshake is complete, we poll on whatever we
need. However, SSL can renegotiate at any time,
so we also merge with what it needs.
In addition, we avoid the unnecessary read when
poll doesn't give us POLLIN in revents, since the
read will more likely than not fail (except in
the rare case when data becomes available in the
interim). Notice that SSL_read will return
SSL_WANTS_READ when there is no data, which
is misleading (since SSL isn't in need of data to
read at all, nor are we, for that matter).
Best not to do noisy reads unnecessarily.
These changes are disabled by default and can
be enabled via the experimental_features option.
Change-Id: I6a7ed7d871ed257b30062cc720a8b8c7acbab3b7
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
This prevents the kit from becoming a zombie
when it has an early failure to connect to WSD.
Change-Id: I4e8203b2cc3d80822308236e5b47be0c0c96e9ae
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
In 'debug' log-level we expect a detailed, but
still readable output. Having one area with
disproportionately large number of logs reduces
the overall utility of the log output.
This patch reduces a number of redundant log
entries, including errors that are already
logged. It also reduces the level of some
others from 'information' to 'debug' and
from 'debug' to 'trace'.
The goal is to make 'debug' level as useful as
possible to read the progress and be able to
understand what was going on, such that one is
able to decide which area to dig deeper into.
Then, trace level could be used to get more
insight into that area, if necessary. For
example, when investigating a test failure,
one first enables 'debug' logs and reads through.
Once a section between two debug entries is
identified as being of interest, enabling 'trace'
level logs becomes more productive as it's
now possible to easily reach the first DBG
entry and read through until the second one.
It's unfortunate that we don't have per-area
control for enabling/disabling logs, so it
is common to see more and more 'debug' log
entries added all around, making logs
less and less readable.
It is also a limitation of the levels we have
that we really only have 3 usable levels:
one, two, many. That is, 'information' for
the most important events, 'debug' for
technical details needed to investigate issues,
and 'trace' for everything else. ('warning'
and 'error' aren't really 'levels'; they have
semantics that makes them special-cases.)
So we have to avoid degrading one into the
other, or have differences without distinction.
If any of these entries are needed to be
displayed more frequently, changing them
back to 'debug' or even 'information' should
be done. Though for me they seem special
cases that don't benefit most log readings.
Change-Id: Id2c6a9dc027483b81a066b0b4b50a298c5eff449
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Michael Meeks
Caolán McNamara
Ashod Nakashian
Miklos Vajna
Shehr Bano
Andras Timar