| Date | Commit message (Collapse) |
|---|
|
We don't want to get hung into a state where we see "\n" via
index(), yet cannot consume rbuf in the while loop. So tweak
the regexp to ensure we always consume rbuf.
I suspect this is what causes occasional 100% CPU usage of
-nntpd, but reproducing it's been difficult..
|
|
Since Net::NNTP::listgroup doesn't support the range parameter,
I had to test this manually and noticed extra CRLF were emitted.
|
|
RFC3977 6.1.2.2 LISTGROUP allows a [range] arg after [group],
and supporting it allows NNTP support in neomutt to work again.
Tested with NeoMutt 20170113 (1.7.2) on Debian stretch
(oldstable)
|
|
RFC3977 8.4.2 mandates the order of non-standard headers
to be after the first seven standard headers/metadata;
so "Xref:" must appear after "Lines:"|":lines".
Additionally, non-required header names must be followed
by ":full".
Cc: Jonathan Corbet <corbet@lwn.net>
Reported-by: Urs Janßen
<E1hmKBw-0008Bq-8t@akw>
|
|
We need to ensure further timers can be registered if there's
currently no idle clients.
|
|
In HTTP.pm, we can use the same technique NNTP.pm uses with
long_response with the $long_cb callback and avoid storing
$pull in the per-client structure at all. We can also reuse
the same logic to push the callback into wbuf from NNTP.
This does NOT introduce a new circular reference, but documents
it more clearly.
|
|
Relying on "use" to import during BEGIN means we get to take
advantage of prototype checking of function args during the rest
of the compilation phase.
|
|
Add some checks for errors at initialization, though there's not
much that can be done with ENOMEM-type errors aside from
dropping clients.
We can also get rid of the scary FIXME for MemLevel=8. It was a
stupid error on my part in the original per-client deflate
stream implementation calling C::R::Z::{Inflate,Deflate} in
array context and getting the extra dualvar error code as a
string result, causing the {zin}/{zout} array refs to have
extra array elements.
|
|
Using Z_FULL_FLUSH at the right places in our event loop, it
appears we can share a single zlib deflate context across ALL
clients in a process.
The zlib deflate context is the biggest factor in per-client
memory use, so being able to share that across many clients
results in a large memory savings.
With 10K idle-but-did-something NNTP clients connected to a
single process on a 64-bit system, TLS+DEFLATE used around
1.8 GB of RSS before this change. It now uses around 300 MB.
TLS via IO::Socket::SSL alone uses <200MB in the same situation,
so the actual memory reduction is over 10x.
This makes compression less efficient and bandwidth increases
around 45% in informal testing, but it's far better than no
compression at all. It's likely around the same level of
compression gzip gives on the HTTP side.
Security implications with TLS? I don't know, but I don't
really care, either... public-inbox-nntpd doesn't support
authentication and it's up to the client to enable compression.
It's not too different than Varnish caching gzipped responses
on the HTTP side and having responses go to multiple HTTPS
clients.
|
|
This is only tested so far with my patches to Net::NNTP at:
https://rt.cpan.org/Ticket/Display.html?id=129967
Memory use in C10K situations is disappointing, but that's
the nature of compression.
gzip compression over HTTPS does have the advantage of not
keeping zlib streams open when clients are idle, at the
cost of worse compression.
|
|
It'll be accessible from other places, and there was no real
point in having it declared inside a sub.
|
|
It's a tad slower, but we'll be able to subclass this to rely
on zlib deflate buffering. This is advantageous for TLS clients
since (AFAIK) IO::Socket::SSL/OpenSSL doesn't give us ways to use
MSG_MORE or writev(2) like like GNUTLS does.
|
|
Some clients may rely on this for STARTTLS support.
|
|
Before I figured out the long_response API, I figured there'd
be expensive, process-monopolizing commands which admins might
want to disable. Nearly 4 years later, we've never needed it
and running a server without commands such as OVER/XOVER is
unimaginable.
|
|
* origin/email-simple-mem:
nntp: reduce syscalls for ARTICLE and BODY
mbox: split header and body processing
mbox: use Email::Simple->new to do in-place modifications
nntp: rework and simplify art_lookup response
|
|
We need to be careful about handling EAGAIN on write(2)
failures deal with SSL_WANT_READ vs SSL_WANT_WRITE as
appropriate.
|
|
Our hacks in EvCleanup::next_tick and EvCleanup::asap were due
to the fact "closed" sockets were deferred and could not wake
up the event loop, causing certain actions to be delayed until
an event fired.
Instead, ensure we don't sleep if there are pending sockets to
close.
We can then remove most of the EvCleanup stuff
While we're at it, split out immediate timer handling into a
separate array so we don't need to deal with time calculations
for the event loop.
|
|
We'll be reusing requeue in other places to reduce trips to
the kernel to retrieve "hot" descriptors.
|
|
Doing this for HTTP cuts the memory usage of 10K
idle-after-one-request HTTP clients from 92 MB to 47 MB.
The savings over the equivalent NNTP change in commit
6f173864f5acac89769a67739b8c377510711d49,
("nntp: lazily allocate and stash rbuf") seems down to the
size of HTTP requests and the fact HTTP is a client-sends-first
protocol where as NNTP is server-sends-first.
|
|
Chances are we already have extra buffer space following the
expensive LF => CRLF conversion that we can safely append an
extra CRLF in those places without incurring a copy of the
full string buffer.
While we're at it, document where our pain points are in terms
of memory usage, since tracking/controlling memory use isn't
exactly obvious in high-level languages.
Perhaps we should start storing messages in git as CRLF...
|
|
We don't need some of the array elements returned from
art_lookup, anymore (and haven't used them in years).
We can also shorten the lifetime of the Email::Simple object by
relying on the fact Email::Simple->new will modify it's arg if
given a SCALARREF and allow us to avoid Email::Simple::body
calls.
Unfortunately, this doesn't seem to provide any noticeable
improvement in memory usage when dealing with a 30+ MB test
message, since our previous use of ->body_set('') was saving
some memory, but forcing a LF-only body to be CRLF was making
Perl allocate extra space for s///sg.
|
|
A tiny write() for the greeting on a just accept()-ed TCP socket
won't fail with EAGAIN, so we can avoid the extra epoll syscall
traffic with plain sockets.
|
|
Allocating a per-client buffer up front is unnecessary and
wastes a hash slot. For the majority of (non-malicious)
clients, we won't need to store rbuf in a long-lived object
associated with a client socket at all.
This saves around 10M on 64-bit with 20K connected-but-idle
clients.
|
|
We can get rid of the {long_res} field and reuse the write
buffer ordering logic to prevent nesting of responses from
requeue.
On FreeBSD, this fixes a problem of callbacks firing twice
because kqueue as event_step is now our only callback entry
point.
There's a slight change in the stdout "logging" format, in
that we can no longer distinguish between writes blocked
due to slow clients or deferred long responses. Not sure
if this affects anybody parsing logs or not, but preserving
the old format could prove expensive and not worth the
effort.
|
|
No need to allocate a new PerlIO::scalar filehandle for every
client, instead we can now pass the same CODE reference which
calls DS->write on a reused string reference.
|
|
This is in accordance with TLS standards and will be needed
to support session caching/reuse in the future. However, we
don't issue shutdown(2) since we know not to inadvertantly
share our sockets with other processes.
|
|
IO::Socket::SSL will try to re-bless back to the original class
on TLS negotiation failure. Unfortunately, the original class
is 'GLOB', and re-blessing to 'GLOB' takes away all the IO::Handle
methods, because Filehandle/IO are a special case in Perl5.
Anyways, since we already use syswrite() and sysread() as functions
on our socket, we might as well use CORE::close(), as well (and
it plays nicely with tied classes).
|
|
It kinda, barely works, and I'm most happy I got it working
without any modifications to the main NNTP::event_step callback
thanks to the DS->write(CODE) support we inherited from
Danga::Socket.
|
|
This will be needed for NNTPS support, since we need
to negotiate the TLS connection before writing the
greeting and we can reuse the existing buffer layer
to enqueue writes.
|
|
We can be smarter about requeuing clients to run and avoid
excessive epoll_ctl calls since we can trust event_step to do
the right thing depending on the state of the client.
|
|
Both NNTP and HTTP have common needs and we can factor
out some common code to make dealing with IO::Socket::SSL
easier.
|
|
It should not matter because our rbuf is always from
a socket without encoding layers, but this makes things
easier to follow.
|
|
This is cleaner in most cases and may allow Perl to reuse memory
from unused fields.
We can do this now that we no longer support Perl 5.8; since
Danga::Socket was written with struct-like pseudo-hash support
in mind, and Perl 5.9+ dropped support for pseudo-hashes over
a decade ago.
|
|
Integer comparisions of "$!" are faster than hash lookups.
See commit 6fa2b29fcd0477d126ebb7db7f97b334f74bbcbc
("ds: cleanup Errno imports and favor constant comparisons")
for benchmarks.
|
|
We don't need to keep track of that field since we always
know what events we're interested in when using one-shot
wakeups.
|
|
We can avoid the EPOLL_CTL_ADD && EPOLL_CTL_MOD sequence with
a single EPOLL_CTL_ADD.
|
|
No sense in having similar Linux-specific functionality in
both our NNTP.pm and HTTP.pm
|
|
We don't need write buffering unless we encounter slow clients
requesting large responses. So don't waste a hash slot or
(empty) arrayref for it.
|
|
All of our internal timing code should use monotonic clocks
for consistency against system clock adjustments.
This can be shared by our Daemon and NNTP packages.
|
|
Merely checking the presence of the {sock} field is
enough, and having multiple sources of truth increases
confusion and the likelyhood of bugs.
|
|
Having separate read/write callbacks in every class is too
confusing to my easily-confused mind. Instead, give every class
an "event_step" callback which is easier to wrap my head around.
This will make future code to support IO::Socket::SSL-wrapped
sockets easier-to-digest, since SSL_write() can require waiting
on POLLIN events, and SSL_read() can require waiting on POLLOUT
events.
|
|
* origin/ds:
ds: stop caring about event flags set by epoll/poll/kqueue
ds: do not distinguish between POLLHUP and POLLERR
ds: remove read method, here, too
nntp: use sysread to append to existing buffer
ds: remove steal_socket method
ds: remove {fd} field
ds: reduce Errno imports and drop ->close reason
ds: cleanup Errno imports and favor constant comparisons
ds: simplify write buffer accounting
|
|
It's the unfortunate reality that there are some clients which
reuse Message-IDs (in which we generate + use another) or set
multiple Message-IDs on their own. While the v2 format
addresses that, NNTP clients such as leafnode are not always
prepared to deal with that case.
So, ensure NNTP clients only see a single Message-ID, and
show the others as 'X-Alt-Message-ID'.
|
|
Leafnode cannot handle Message-ID headers which are too long and
require folding via Email::Simple::Header. Since there are
already many of these messages in git with the header already
folded, we need to handle the unfolding when emitting the
message via NNTP.
As far as we know, Leafnode is the only client software
incapable of handling this case.
|
|
Apparently leafnode just needs any junk in the Path: header.
Lets not waste bandwidth and just use a single byte to keep
leafnode happy.
Cc: Dave Taht <dave@taht.net>
|
|
In my experience, both are worthless as any normal read/write
call path will be wanting to check errors and deal with them
appropriately; so we can just call event_read, for now.
Eventually, there'll probably be only one callback for dealing
with all in/out/err/hup events to simplify logic, especially w.r.t
TLS socket negotiation.
|
|
We already do this in PublicInbox::HTTP, as it's superior to
DS::read in this regard. Initially (when I started writing
NNTP.pm, I wanted to use Danga::Socket's read buffering and
push_back_read (removed in DS) but quickly figured out it wasn't
useful at all for dealing with trickling clients.
|
|
Storing the file descriptor was redundant as we can quickly call
fileno($self->{sock}) and not have to store an extra hash table
entry. Multiple sources of truth leads to confusion, confusion
leads to bugs.
|
|
Keeping track of write_buf_size was redundant and pointless when
we can simply check the number of elements in the buffer array.
Multiple sources of truth leads to confusion; confusion leads to
bugs.
Finally, rename the prefixes to 'wbuf' to ensure we loudly
(instead of silently) break any external dependencies being
ported over from Danga::Socket, as further changes are pending.
|
|
RFC3977 does not have provisions for whitespace beyond ASCII
TAB, SP, CR and LF. I doubt there's any NNTP clients broken
enough to be sending non-ASCII whitespace delimiters.
We're probably excessively liberal regarding TAB acceptance,
even; but it's probably too late to change at this point...
|