Eric Wong <[email protected]> wrote:
> The on-disk storage aspect still has me a little nervous :x
I think this needs to be squashed into [PATCH 1/2] to avoid
migration costs (probably nobody but me is running this, yet):
------8<-------
Subject: [PATCH] pop3: fix numerous bugs in delete handling
This requires a DB change to simplify our Perl code, but there's
no migration for it presently.
We were also incorrectly relying on array offsets instead of sequence
numbers for "EXPIRE 0" handling.
---
lib/PublicInbox/POP3.pm | 4 ++--
lib/PublicInbox/POP3D.pm | 2 +-
t/pop3d.t | 37 +++++++++++++++++++++++++++++++++++--
3 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/lib/PublicInbox/POP3.pm b/lib/PublicInbox/POP3.pm
index 5cffc556..51c2b71a 100644
--- a/lib/PublicInbox/POP3.pm
+++ b/lib/PublicInbox/POP3.pm
@@ -218,7 +218,7 @@ sub need_txn ($) {
sub _stat_cache ($) {
my ($self) = @_;
- my ($beg, $end) = ($self->{uid_dele} // 0, $self->{uid_max});
+ my ($beg, $end) = (($self->{uid_dele} // -1) + 1, $self->{uid_max});
PublicInbox::IMAP::uid_clamp($self, \$beg, \$end);
my $opt = { limit => PublicInbox::IMAP::UID_SLICE };
my $m = $self->{ibx}->over(1)->do_get(<<'', $opt, $beg, $end);
@@ -305,7 +305,7 @@ sub retr_cb { # called by git->cat_async via ibx_async_cat
$$bref .= substr($$bref, -2, 2) eq "\r\n" ? ".\r\n" : "\r\n.\r\n";
$self->msg_more("+OK message follows\r\n");
$self->write($bref);
- $self->{expire} .= pack('S', $off) if exists $self->{expire};
+ $self->{expire} .= pack('S', $off + 1) if exists $self->{expire};
$self->requeue;
}
diff --git a/lib/PublicInbox/POP3D.pm b/lib/PublicInbox/POP3D.pm
index 65997f6d..c7bf1755 100644
--- a/lib/PublicInbox/POP3D.pm
+++ b/lib/PublicInbox/POP3D.pm
@@ -84,7 +84,7 @@ CREATE TABLE IF NOT EXISTS deletes (
txn_id INTEGER PRIMARY KEY NOT NULL, /* -1 == txn lock offset */
user_id INTEGER NOT NULL REFERENCES users,
mailbox_id INTEGER NOT NULL REFERENCES mailboxes,
- uid_dele INTEGER NULL, /* IMAP UID, NNTP article number */
+ uid_dele INTEGER NOT NULL DEFAULT -1, /* IMAP UID, NNTP article */
UNIQUE(user_id, mailbox_id) )
}
diff --git a/t/pop3d.t b/t/pop3d.t
index e781987e..d5ccb0d8 100644
--- a/t/pop3d.t
+++ b/t/pop3d.t
@@ -54,10 +54,10 @@ unless (-r $key && -r $cert) {
my $old = start_script(['-pop3d', '-W0',
"--stdout=$tmpdir/plain.out", "--stderr=$olderr" ],
$env, { 3 => $plain });
-my $oldc = Net::POP3->new($plain->sockhost, Port => $plain->sockport);
+my @old_args = ($plain->sockhost, Port => $plain->sockport);
+my $oldc = Net::POP3->new(@old_args);
my $locked_mb = ('e'x32)."\@$group";
ok($oldc->apop("$locked_mb.0", 'anonymous'), 'APOP to old');
-my @old_args = ($plain->sockhost, Port => $plain->sockport);
{ # locking within the same process
my $x = Net::POP3->new(@old_args);
@@ -238,6 +238,39 @@ EOF
{
my $capa = $oldc->capa;
ok(defined($capa->{PIPELINING}), 'pipelining supported by CAPA');
+ is($capa->{EXPIRE}, 0, 'EXPIRE 0 set');
+
+ # clients which see "EXPIRE 0" can elide DELE requests
+ my $list = $oldc->list;
+ ok($oldc->get($_), "RETR $_") for keys %$list;
+ ok($oldc->quit, 'QUIT after RETR');
+
+ $oldc = Net::POP3->new(@old_args);
+ ok($oldc->apop("$locked_mb.0", 'anonymous'), 'APOP reconnect');
+ my $cont = $oldc->list;
+ is_deeply($cont, {}, 'no messages after implicit DELE from EXPIRE 0');
+ ok($oldc->quit, 'QUIT on noop');
+
+ # test w/o checking CAPA to trigger EXPIRE 0
+ $oldc = Net::POP3->new(@old_args);
+ ok($oldc->apop($locked_mb, 'anonymous'), 'APOP on latest slice');
+ my $l2 = $oldc->list;
+ is_deeply($l2, $list, 'different mailbox, different deletes');
+ ok($oldc->get($_), "RETR $_") for keys %$list;
+ ok($oldc->quit, 'QUIT w/o EXPIRE nor DELE');
+
+ $oldc = Net::POP3->new(@old_args);
+ ok($oldc->apop($locked_mb, 'anonymous'), 'APOP again on latest');
+ $l2 = $oldc->list;
+ is_deeply($l2, $list, 'no DELE nor EXPIRE preserves messages');
+ ok($oldc->delete(2), 'explicit DELE on latest');
+ ok($oldc->quit, 'QUIT w/ highest DELE');
+
+ # this is non-standard behavior, but necessary if we expect hundreds
+ # of thousands of users on cheap HW
+ $oldc = Net::POP3->new(@old_args);
+ ok($oldc->apop($locked_mb, 'anonymous'), 'APOP yet again on latest');
+ is_deeply($oldc->list, {}, 'highest DELE deletes older messages, too');
}
# TODO: more tests, but mpop was really helpful in helping me
