memcached_vdo() 함수의 오류 처리 일관성 보장.

[jhpark816]

memcached_vdo() 함수의 오류 처리 부분이 일관적인지 검토하고, 필요하면 수정한다.

[uhm0311]

@jhpark816

arcus-c-client/libmemcached/do.cc

Lines 80 to 85 in 71bcafd

	if (memcached_failed(rc= memcached_connect(ptr)))
	{
	WATCHPOINT_ERROR(rc);
	assert_msg(ptr->error_messages, "memcached_connect() returned an error but the memcached_server_write_instance_st showed none.");
	return rc;
	}

arcus-c-client/libmemcached/do.cc

Lines 105 to 116 in 71bcafd

	if (sent_length == -1 or size_t(sent_length) != command_length)
	{
	rc= MEMCACHED_WRITE_FAILURE;
	WATCHPOINT_ERROR(rc);
	WATCHPOINT_ERRNO(errno);
	}
	else if ((ptr->root->flags.no_reply) == 0 and (ptr->root->flags.piped == false))
	{
	memcached_server_response_increment(ptr);
	}
	return rc;

위 쪽 코드와 아래 쪽 코드의 오류 처리에 일관성이 떨어지는 것 같습니다. 아래 쪽 코드를 다음과 같이 수정하면 좋을 것 같습니다.

memcached_return_t rc= MEMCACHED_SUCCESS;

...

if (sent_length == -1 or size_t(sent_length) != command_length)
{
  rc= MEMCACHED_WRITE_FAILURE;
  WATCHPOINT_ERROR(rc);
  WATCHPOINT_ERRNO(errno);
  return rc;
}

if ((ptr->root->flags.no_reply) == 0 and (ptr->root->flags.piped == false))
{
  memcached_server_response_increment(ptr);
}

return rc;

[jhpark816]

@uhm0311

• memcached_vdo() 함수를 호출하는 부분에서 오류 처리가 일관적인지를 봐 주세요.
• memcached_vdo() 함수 구현체는 문제가 없으므로 변경하지 않을 것입니다.

[jhpark816]

@uhm0311
PR 올린 거는 우선순위를 뒤로 미루어 살펴보도록 할게요.

[jhpark816]

@uhm0311

• memcached_vdo() 함수의 리턴 값을 모두 정리하고, 리턴 값에 대한 처리 방식을 먼저 정리함으로써
  본 이슈의 처리 방식에 대한 공통 의견을 모으는 것이 우선일 것 같습니다.
  • 본 이슈의 코멘트로 정리하면 될 것 같습니다.
• 그리고 나서, 리턴 값에 대한 처리 코드의 구현을 동일하게 정리하는 순서로 진행해 보시죠.
  • 모든 경우에서 동일하게 1개 pattern의 코드로 정리되면 좋겠지만,
    여러 경우가 있어서 2~3개 pattern이 될 수도 있을 것 같습니다.
  • 여튼, 이 부분은 위의 첫째 사항 정리 후에 논의하시죠.

[uhm0311]

@jhpark816

• 모든 return 값 목록입니다.

MEMCACHED_SUCCESS : 성공
MEMCACHED_CONNECTION_FAILURE
MEMCACHED_ERRNO
MEMCACHED_FAILURE
MEMCACHED_HOST_LOOKUP_FAILURE
MEMCACHED_INVALID_ARGUMENTS
MEMCACHED_MEMORY_ALLOCATION_FAILURE
MEMCACHED_NOT_SUPPORTED
MEMCACHED_SERVER_MARKED_DEAD
MEMCACHED_SERVER_TEMPORARILY_DISABLED : disconneted 상태로, reconnect 까지 요청 처리 불가 상태
MEMCACHED_TIMEOUT
MEMCACHED_WRITE_FAILURE : write() 수행 오류

의사 코드로 나타낸 모든 return 값

memcached_vdo() {
  memcached_connect() {
    backoff_handling() {
      return MEMCACHED_SERVER_MARKED_DEAD // server->server_failure_counter >= server->root->server_failure_limit이고 mc->flags.auto_eject_hosts == true인 경우 
      return MEMCACHED_SERVER_TEMPORARILY_DISABLED // server->state == MEMCACHED_SERVER_STATE_IN_TIMEOUT이고 retry time 조건을 만족하지 않는 경우
      return MEMCACHED_SUCCESS // 성공
    }
    network_connect() {
      set_hostinfo() {
        return MEMCACHED_FAILURE // server->port를 string으로 변환하지 못한 경우. hash collision 이슈 PR로 str_port를 미리 들고 있게 되어 고려하지 않아도 됨
        return MEMCACHED_TIMEOUT // getaddrinfo() 함수의 return 값이 EAI_AGAIN인 경우
        return MEMCACHED_ERRNO // getaddrinfo() 함수의 return 값이 EAI_SYSTEM인 경우
        return MEMCACHED_INVALID_ARGUMENTS // getaddrinfo() 함수의 return 값이 EAI_BADFLAGS인 경우
        return MEMCACHED_MEMORY_ALLOCATION_FAILURE // getaddrinfo() 함수의 return 값이 EAI_MEMORY인 경우
        return MEMCACHED_HOST_LOOKUP_FAILURE // getaddrinfo() 함수의 return 값이 위의 4가지가 아닌 경우
        return MEMCACHED_SUCCESS // 성공
      }
      connect_poll() {
        return MEMCACHED_ERRNO // poll() 함수의 return 값이 1이고 getsockopt() 함수의 errcode가 0이 아닌 경우, 혹은 poll() 함수의 return 값이 0이나 1이 아니고 errno가 ERESTART, EINTR, EFAULT, ENOMEM, EINVAL이 아닌 경우
        return MEMCACHED_TIMEOUT // mc->poll_timeout == 0인 경우, 혹은 poll() 함수의 return 값이 0인 경우
        return MEMCACHED_SUCCESS // 성공
        return MEMCACHED_MEMORY_ALLOCATION_FAILURE // poll() 함수의 return 값이 0, 1이 아니고 errno가 EFAULT, ENOMEM, EINVAL인 경우
      }
      return MEMCACHED_SUCCESS // 성공
      return MEMCACHED_TIMEOUT // connect() 함수의 return 값이 SOCKET_ERROR이고 errno가 ETIMEDOUT인 경우, 혹은 connect_poll() 함수의 return 값이 MEMCACHED_TIMEOUT이고 server->state 값이 MEMCACHED_SERVER_STATE_NEW, MEMCACHED_SERVER_STATE_ADDRINFO인 경우
      return MEMCACHED_CONNECTION_FAILURE // connect() 함수의 return 값이 SOCKET_ERROR이고 errno가 ETIMEDOUT, EWOULDBLOCK, EINPROGRESS, EALREADY, EINTR가 아닌 경우
      return MEMCACHED_ERRNO // socket() 함수의 return 값이 음수인 경우
    }
    unix_socket_connect() {
      return MEMCACHED_CONNECTION_FAILURE // socket() 함수의 return 값이 음수인 경우, 혹은 socket() 함수의 return 값이 음수가 아니고 connect() 함수의 return 값이 음수이면서 errno가 EINPROGRESS, EALREADY, EINTR, EISCONN이 아닌 경우
      return MEMCACHED_SUCCESS // 성공
    }
    memcached_version_instance() {
      version_binary_instance() {
        return memcached_vdo()
      }
      version_ascii_instance() {
        return memcached_vdo()
      }
      return MEMCACHED_NOT_SUPPORTED // 연결이 정상적으로 완료되었으나 mc->flags.use_udp == true인 경우
      return MEMCACHED_INVALID_ARGUMENTS // mc == NULL 혹은 server == NULL인 경우
    }
    return MEMCACHED_SERVER_TEMPORARILY_DISABLED // backoff_handling() 함수에서 server->state == MEMCACHED_SERVER_STATE_IN_TIMEOUT이고 retry time 조건을 만족하여 timeout 상태이지만 backoff_handling() 함수의 reutrn 값이 MEMCACHED_SUCCESS인 경우
  }
  return MEMCACHED_WRITE_FAILURE // memcached_io_writev() 함수가 실패하여 return 값이 -1이거나 write를 했으나 일부는 write가 이뤄지지 않은 경우
}

• memcached_connect() 함수 안에서 memcached_sasl_authenticate_connection() 함수를 호출하는데, binary protocol인 경우에만 동작하므로 고려하지 않았습니다.
• memcached_vdo() 함수가 실패하는 경우는 연결이 올바로 수립되지 않았거나(memcached_connect() 함수가 실패), socket에 write가 정상적으로 수행되지 않은 경우(return 값이 MEMCACHED_WRITE_FAILURE인 경우)입니다.
• return 값이 MEMCACHED_WRITE_FAILURE이면 연결이 수립되었단 뜻이므로, 연결을 해제하기 위해 memcached_io_reset() 함수를 호출하면 될 것 같습니다.
• 그 외의 return 값은 연결이 수립되지 않아서 실패한 경우이므로 후속 조치를 취하지 않아도 될 것 같습니다.
• memcached_version_instance() 함수에서 발생하는 에러 코드 MEMCACHED_NOT_SUPPORTED, MEMCACHED_INVALID_ARGUMENTS는 함수를 정상적으로 호출했을 경우엔 나타나지 않습니다. 휴먼 에러를 막기 위해 있는 것으로 보입니다.
• memcached_version_instance() 함수의 오류 처리도 memcached_io_reset() 함수를 호출하면 될 것 같습니다.

[jhpark816]

@uhm0311
같은 오류 코드가 여러 군데서 설정되므로, 오류 코드만 보고 error handling 정하기가 어려운 것 같네요.
오류 코드만 보고 어떤 상황인지를 unique하게 구별하도록 오류 설정 부분을 먼저 수정해야 하는 것이 아닌가요?

[uhm0311]

@jhpark816
응용에서 오류 코드를 보고 에러 핸들링을 할 때 말씀이신가요?

[jhpark816]

@uhm0311
memcached_vdo()는 내부 함수입니다.
이를 호출하는 arcus-c-client 내부에서 해당 오류 코드를 보고 error handling 정확히 하려는 것입니다.

예를 들어, MEMCACHED_INVALID_ARGUMENTS 오류인 경우,
아래 2 군데 중 어떤 실패인 지에 따라 error handling이 달라져야 할 것 같습니다.

• set_hostinfo()에서 실패
• memcached_version_instance()에서 실패

따라서, 위 코멘트에 아래 사항이 보강되어야 할 것 같습니다.

• 오류 코드에 대한 상황 설명
• memcached_version_instance() 수행 오류는 connected 상태에서 발생할 것인 데, 이를 어떻게 처리할 것인지?
  • disconnect 처리 or 다른 처리 ?

[uhm0311]

@jhpark816

코멘트 수정했습니다.

[jhpark816]

@uhm0311
모든 return 값 목록에서 오류 코드의 상황 설명을 부탁해요.

[uhm0311]

@jhpark816

중복되는 오류코드가 있어서 각기 다른 상황을 설명하기 위해 의사 코드로 나타낸 모든 return 값에 설명해두었습니다.

[jhpark816]

@uhm0311
현재 상태론 어떤 결정을 내릴 근거가 부족합니다.
보다 명확한 정리가 필요한 것 같습니다.

[uhm0311]

@jhpark816

어떤 정리가 더 필요한지 힌트 주실 수 있을까요?

[jhpark816]

@uhm0311
실패 조건의 판단 방법 2가지 중 어떤 형태가 적합한가요?

[uhm0311]

@jhpark816

memcached_version_instance() 함수의 실패를 따로 관리한다면 != MEMCACHED_SUCCESS로 충분해 보입니다.

[uhm0311]

@jhpark816

작업을 여러 commit으로 나누어 봤습니다.

1. mecmached_vdo() 함수 리팩토링
2. memcached_vdo() 함수를 호출하는 각 소스를 나눠서 커밋 (auto.cc 커밋, collection.cc 커밋, delete.cc 커밋 등)
3. memcached_vdo() 함수 내에서 MEMCACHED_WRITE_FAILURE인 경우 memcached_io_reset() 호출하도록 변경하고 memcached_vdo() 함수 호출하는 부분에서 memcached_io_reset() 호출 제거
4. memcached_version_instance() 함수의 실패 시 처리 추가

[jhpark816]

@uhm0311
아래 순서로 아래 작업을 먼저 진행하시고, 나머지는 다시 보도록 합시다.

• mecmached_vdo() 함수 리팩토링
• memcached_version_instance() 함수의 실패 시 처리 추가

[jhpark816]

@uhm0311 @SuhwanJang
memcached_vdo() 함수의 오류 핸들링이 서로 상이한 상태인 데,
모든 오류 경우를 고려한다면, 어떤 오류 핸들링이 가장 정확한 방안인가요?