INTERNAL: Fix problems about not released items and an assertion in UDP

[ing-eoking]

🔗 Related Issue

• jam2in/arcus-works#529

⌨️ What I did

• UDP에서 connection 재사용을 위해 conn_cleanup 일부 로직을 conn_close로 이동했습니다.
• UDP에서 conn_cleanup 수행 후 conn_read로 바꾸게 끔 했습니다.
• UDP 소켓 실패 시 conn_closing으로 바꾸게 끔 했습니다.

🔍  About Issue

• 기존 로직에서 UDP의 conn_closing 시 발생하는 문제
  1. conn_cleanup 만 수행
  2. c->thread = NULL / c->std = -1 변경 (이벤트에서 제거 X)
  3. 해당 connection에서 새로운 이벤트 발생
  4. 이벤트에 등록된 sfd와 c->sfd(= -1) 차이 발생
  5. conn_close
  6. c->thread = NULL 이므로 assert(c->thread) 로 인한 전체 프로그램 종료
• conn_read로의 변경 이유
  • conn_cleanup 이후 새로운 명령어를 받을 수 있는 state로 변경이 필요
  • read 데이터 존재 X 시 read 이벤트를 등록하는 conn_waiting으로 변경도 필요
  • conn_read : 완전히 새로운 read를 받음
  • conn_new_cmd : read buffer에 남아있는 cmd를 수행 후 새로운 read 수행
  • 에러로 인한 재수행이므로 완전히 새롭게 명령을 수행하는 conn_read가 더 맞다고 생각됩니다.

    conn_cleanup()만 호출하는 경우에는 sfd가 -1로 초기화되어야 하나요?

conn_cleanup 만 수행해야 하는경우 sfd를 초기화해서는 안됩니다.

🧪 Test

• ~Perl 스크립트로 간단한 set 명령어를 날려 확인~
• ~Perl 스크립트를 활용해서 telnet과 유사한 코드를 구현하여 직접 테스트~
  • ~https://github.com/ing-eoking/arcus-telnet-UDP~ (Deprecated)
  • https://github.com/ing-eoking/arcus-cli/blob/main/util/udp
• memcached 로직을 write 이후 무조건 에러로 처리하게 끔 변경 후 서버 상태 및 셧다운 확인

  Assertion failed: (c->thread), function conn_close, file memcached.c, line 938.
  zsh: abort      ./memcached -E .libs/default_engine.so -U 11211

[jhpark816]

UDP 소켓 실패 시 conn_closing으로 바꾸게 끔 했습니다.

이 내용은 해당 이슈에서 설명한 부분이므로 알고 있습니다.

UDP에서 connection 재사용을 위해 conn_cleanup 일부 로직을 conn_close로 이동했습니다. UDP에서 conn_cleanup 수행 후 conn_read로 바꾸게 끔 했습니다. conn_cleanup과 conn_mwrite에서 공통되는 로직(item release 등)을 conn_item_free 함수로 분리했습니다.

리팩토링이 들어간 것 같은 데, 이번 이슈는 버그를 수정하는 것이니 버그 수정하는 최소한의 변경만 합시다.

버그 수정을 merge한 후에 리팩토링은 다시 PR 받을게요.

[ing-eoking]

수정되었습니다.

UDP에서 connection 재사용을 위해 conn_cleanup 일부 로직을 conn_close로 이동했습니다. UDP에서 conn_cleanup 수행 후 conn_read로 바꾸게 끔 했습니다.

이 두 부분 또한 버그에 가깝습니다. (https://github.com/jam2in/arcus-works/issues/529#issuecomment-2062812416)

[jhpark816]

@namsic 리뷰 바랍니다.

[ing-eoking]

UDP 소켓 보낼 때 테스트했던 코드는 아래와 같으나 사실상 너무 단순하게 set i 0 0 1 명령어 보내는 게 다 이며, 실제로는 memcached 코드를 변경시켜서 로그를 확인했습니다. 이 부분도 좀더 보완해서 나중에 다시 올리겠습니다.

#!/usr/bin/perl

use IO::Socket::INET;
my $socket;
my $cmd = "set i 0 0 1\r\n1\r\n";

$socket = new IO::Socket::INET (
PeerAddr   => '127.0.0.1:11211',
Proto      => 'udp'
) or die "ERROR : $!\n";

$reqid = 45;
$hdr = pack("nnnn", $reqid, 0, 1, 0);
my $pkt = $hdr . $cmd;

$socket->send($pkt);

my $got = 0;
my $pkts = 1;
my $resp = "";

while($got < $pkts) {
    my $pkt;
    $sock->recv($pkt, 1500, 0);
    my ($resid, $total, $curr, $zero) = unpack("nnnn", substr($pkt, 0, 8));
    die "A mismatch between response ID and request ID ($reqid != $resid)" unless $resid == $reqid;
    die "Non-zero tail ($zero)" unless $resv == 0;
    $pkts = $total;
    $resp .= substr($pkt, 8);
    $got++;
}

$socket->close();

[jhpark816]

@ing-eoking 본 PR도 진행 바랍니다.

[ing-eoking]

@jhpark816

우선 상단에 conn_cleanup의 로직 이동이 필요한 이유를 추가했습니다.

• 기존 로직에서 UDP의 conn_closing 시 발생하는 문제

[jhpark816]

@ing-eoking 로직 이동이 필요한 이유는 이해했습니다.

이동되는 로직 자체에 대해 리뷰 코멘트가 있으므로, 모든 리뷰 코멘트를 확인하고 처리 or 답변 바랍니다.

[ing-eoking]

@jhpark816.

코멘트에 들어있는 부분은 처리되었습니다.

[ing-eoking]

@jhpark816

답변을 최상단에 추가했습니다.

[jhpark816]

@ing-eoking 최상단에 테스트 방법도 추가해 주시죠.

[ing-eoking]

@ing-eoking 최상단에 테스트 방법도 추가해 주시죠.

수정되었습니다.