Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jhpark816 opened this issue Jul 6, 2021 · 18 comments
Open

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

jhpark816 opened this issue Jul 6, 2021 · 18 comments
Assignees
Labels

Comments

@jhpark816
Copy link
Contributor

jhpark816 commented Jul 6, 2021

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

@uhm0311
Copy link
Collaborator

uhm0311 commented Dec 28, 2021

@jhpark816

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;
}

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;

uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Dec 28, 2021
@jhpark816
Copy link
Contributor Author

jhpark816 commented Dec 28, 2021

@uhm0311

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

uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Dec 29, 2021
@jhpark816
Copy link
Contributor Author

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

uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Mar 7, 2022
@jhpark816
Copy link
Contributor Author

@uhm0311

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

@uhm0311
Copy link
Collaborator

uhm0311 commented Mar 15, 2022

@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
Copy link
Contributor Author

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

@uhm0311
Copy link
Collaborator

uhm0311 commented Mar 15, 2022

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

@jhpark816
Copy link
Contributor Author

@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 added a commit to uhm0311/arcus-c-client that referenced this issue Mar 16, 2022
@uhm0311
Copy link
Collaborator

uhm0311 commented Mar 16, 2022

@jhpark816

코멘트 수정했습니다.

@jhpark816
Copy link
Contributor Author

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

@uhm0311
Copy link
Collaborator

uhm0311 commented Mar 16, 2022

@jhpark816

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

@jhpark816
Copy link
Contributor Author

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

@uhm0311
Copy link
Collaborator

uhm0311 commented Mar 17, 2022

@jhpark816

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

@jhpark816
Copy link
Contributor Author

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

@uhm0311
Copy link
Collaborator

uhm0311 commented Mar 21, 2022

@jhpark816

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

@uhm0311
Copy link
Collaborator

uhm0311 commented Mar 21, 2022

@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
Copy link
Contributor Author

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

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

uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Mar 21, 2022
@jhpark816
Copy link
Contributor Author

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

uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Apr 19, 2022
uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Apr 21, 2022
uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Apr 22, 2022
uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Apr 22, 2022
uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Apr 22, 2022
uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Apr 22, 2022
uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Apr 26, 2022
uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Apr 26, 2022
uhm0311 added a commit to uhm0311/arcus-c-client that referenced this issue Apr 26, 2022
jhpark816 added a commit that referenced this issue Apr 26, 2022
…ling

INTERNAL: Move io reset from where calls memcached_vdo() to where fails insied of memcached_vdo() #167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants