INTERNAL: Refactor textual_value_fetch logic by ing-eoking · Pull Request #390 · naver/arcus-c-client

ing-eoking · 2025-09-25T09:18:00Z

🔗 Related Issue

jam2in/arcus-works#745

⌨️ What I did

textual_value_fetch 로직을 리팩토링했습니다.

UDP 프로토콜 관련 예외 처리
- 응답 크기가 1400바이트를 초과할 수 있는 경우, UDP 프로토콜에서는 이를 지원하지 않아야 하므로 기존의 아래 조건문은 그대로 유지했습니다
```
if (ptr->root->flags.use_udp)
  return memcached_set_error(*ptr, MEMCACHED_NOT_SUPPORTED, MEMCACHED_AT);
```
응답 파싱 방식 개선
- 기존에는 응답 문자열을 띄어쓰기를 기준으로 하나씩 읽어오며 데이터를 추출하고, 그 과정에서 매번 에러 처리를 수행했습니다.
- 이 방식은 반복적으로 중복 코드가 발생하는 문제가 있었습니다.
변경 사항 요약
- 응답 문자열 전체를 한 번에 띄어쓰기를 기준으로 토큰화하여 char * 배열에 저장하도록 변경했습니다.
- 데이터 추출은 이 배열을 순회하면서 수행하며, 토큰화 시점에서 한 번만 에러 처리를 하도록 했습니다.
- 중복된 에러 처리 코드를 제거했습니다.
- 일부 로직은 개인적으로 가독성과 유지보수성을 고려해 보다 나은 형태로 개선했습니다.

ing-eoking · 2025-09-26T08:42:50Z

@jhpark816

리뷰 부탁드립니다.

jhpark816

일부 리뷰

jhpark816 · 2025-09-27T12:05:45Z

+    cur_ptr= str_ptr;
+    while (!iscntrl(*cur_ptr) && !isspace(*cur_ptr))
+    {
+      if (cur_ptr >= end_ptr) return MEMCACHED_PARTIAL_READ;


" abc def" 값을 가정하면, MEMCACHED_PARTIAL_READ 오류가 발생할 것 같습니다.
마지막 charactor는 반드시 control 또는 space charactor이어야 하나요?

iscntrl() 문자가 아니어야 하는 이유가 무엇이라 생각하나요?

\r, \n 문자는 isspace()로 검사가 가능하네요.
문자열 끝을 나타내는 \0(Null) 문자는 control 문자입니다.

우선, 응답으로 "abc def"와 같은 문자열 형태나 \0이 포함되지 않는다는 전제를 바탕으로 코드가 구현되어 있습니다.
해당 문자열은 서버에서 전송되는 값으로, 반드시 VALUE <key> <flags> <bytes> [<cas unique>]\r\n 형식을 따른다고 가정하고 parsing을 수행합니다.

말씀하신 경우들은 극히 드문 상황에 대한 프로토콜 에러에 가깝습니다.
다만, 기존 코드에서는 이러한 상황을 별도로 처리하고 있지 않습니다.

tokenize() 용어의 함수는 tokernize하는 역할을 수행하는 것입니다.
token 수를 리턴하는 것이 좋은 인터페이스이고,
MEMCACHED_PARTIAL_READ를 리턴하는 것은 상식적이지 않습니다.

jhpark816

이전 리뷰에 대해 수정이 이루어지지 않은 것 같습니다.
확인 바랍니다.

jhpark816 · 2025-10-01T02:04:30Z

+    cur_ptr= str_ptr;
+    while (!iscntrl(*cur_ptr) && !isspace(*cur_ptr))
+    {
+      if (cur_ptr >= end_ptr) return MEMCACHED_PARTIAL_READ;


tokenize() 용어의 함수는 tokernize하는 역할을 수행하는 것입니다.
token 수를 리턴하는 것이 좋은 인터페이스이고,
MEMCACHED_PARTIAL_READ를 리턴하는 것은 상식적이지 않습니다.

jhpark816

일부 리뷰

jhpark816

리뷰 완료

jhpark816

리뷰 완료

jhpark816 · 2025-10-09T10:42:04Z

-      result->key_length= key_length;
-    } else {
-      snprintf(key + MEMCACHED_MAX_KEY - 4, 4, "...");
-      result->key_length= MEMCACHED_MAX_KEY - 1;


키 문자열을 읽어내는 부분은 기존 코드를 그대로 유지합시다.
키 문자열의 tokenize와 숫자형 문자열의 tokenize 방식이 다르고,
키 문자열의 tokenize하는 기존 코드가 충분히 이해하기 쉽기 때문입니다.

jhpark816 · 2025-10-09T10:43:38Z

-    goto read_error;
-
-  for (next_ptr= string_ptr; isdigit(*string_ptr); string_ptr++) {};
-  result->item_flags= (uint32_t) strtoul(next_ptr, &string_ptr, 10);


숫자형의 문자열만을 토큰화하도록 합시다.
기존처럼 isdigit() 함수를 이용하여 tokenize 한다면, strtoul() 후에 validation check는 없어도 됩니다.

int tokenize_numbers(char *buffer, size_t length, char **tokens, size_t max_tokens)

토큰화 방식을 활용해 리팩토링을 진행하는 경우, 결과에서 key 부분만 따로 파싱하고 나머지 부분은 토큰화하여 처리하는 방식은 개인적으로는 다소 부자연스럽게 느껴집니다.

java-client의 구현 방식을 살펴본 결과, 전체 결과를 모두 토큰화한 뒤 파싱하는 방식을 사용하고 있었습니다.

https://github.com/naver/arcus-java-client/blob/79c196bf633249b176f7fdbb6f09a22c91d29a18/src/main/java/net/spy/memcached/protocol/ascii/BaseGetOpImpl.java#L87-L101

jhpark816 · 2025-10-09T10:52:08Z

+  size_t ntoken;
+  size_t end_idx;
+  size_t start_idx= 0;
+  for (ntoken= 0; ntoken < max_tokens; ntoken++)


length 기준의 loop를 사용하고, 아래 중의 하나가 만족 될 때까지 tokenize하면 좋겠습니다.

new line이 나올 때까지

length까지 다다를 때까지

max_tokens 만큼 tokenize할 때까지

jhpark816 · 2025-12-18T04:00:04Z

@ing-eoking
PR 수정 코드를 현재 기준으로 다시 확인해 주세요.
그리고, develop 브랜치 기준으로 PR 보내주세요.

ing-eoking requested review from jhpark816 and namsic September 25, 2025 09:18

namsic reviewed Sep 26, 2025

View reviewed changes

Comment thread libmemcached/response.cc Outdated

Comment thread libmemcached/response.cc Outdated

Comment thread libmemcached/response.cc Outdated

Comment thread libmemcached/response.cc Outdated

ing-eoking force-pushed the fetch branch 2 times, most recently from 672ab98 to 3668cab Compare September 26, 2025 08:32

namsic approved these changes Sep 26, 2025

View reviewed changes

jhpark816 reviewed Sep 27, 2025

View reviewed changes

jhpark816 reviewed Oct 1, 2025

View reviewed changes

ing-eoking force-pushed the fetch branch from 3668cab to 5e6c677 Compare October 1, 2025 05:10

jhpark816 reviewed Oct 1, 2025

View reviewed changes

Comment thread libmemcached/response.cc

ing-eoking force-pushed the fetch branch from 5e6c677 to 01ace15 Compare October 2, 2025 00:16

jhpark816 reviewed Oct 2, 2025

View reviewed changes

ing-eoking force-pushed the fetch branch 3 times, most recently from ccb5a0a to 9e9a908 Compare October 2, 2025 08:13

jhpark816 reviewed Oct 9, 2025

View reviewed changes

ing-eoking force-pushed the fetch branch 2 times, most recently from 4597f99 to da4fa79 Compare October 13, 2025 01:18

INTERNAL: Refactor textual_value_fetch logic

1ea7295

ing-eoking changed the base branch from master to develop December 18, 2025 06:55

ing-eoking force-pushed the fetch branch from da4fa79 to 1ea7295 Compare December 18, 2025 06:55

Conversation

ing-eoking commented Sep 25, 2025

🔗 Related Issue

⌨️ What I did

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ing-eoking commented Sep 26, 2025

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

jhpark816 Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhpark816 commented Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jhpark816 Oct 9, 2025 •

edited

Loading