[Feature] Add EventHandler::garbageCollector #39

chaejin-jen · 2023-03-19T01:00:21Z

개요

목적 : close(fd), delete udata 처리

EventHandler 에 garbageCollector 추가
- feat1 : 제한 시간(현재 15초)내에 register 하지 못한 Client 연결 끊기
- feat2 : 연결이 끊긴 Client 삭제
변경사항 handleTimeout, handleClose
- 기존 : handleEvent 에서 사용
- 현재 : garbageCollector 에서 사용

작업내용

close connected_fd : Client::~Client()
delete client(stl) : Executor::deleteClient()
- ChannelController::eraseClient(client)
- ClientController::erase(client->getFd())
delete param : Command::~Command()
delete udata : Server::handleClose()

주의사항

[중요!!!] iterator 로 순회하다가 해당 노드/위치를 delete 해버리면, 해당 순회는 유효하지 않음
- stl 을 사용한 erase 중에 already freed memory에 접근 한다면 의심해보기
renaming 필요
monitorEvent에서 kevent 사용시 Timer(마지막 인자)설정 해줘야 Timeout 제대로 동작
ErrorHandler 오늘 열심히 해볼게요.. 죄송합니닷...
변경사항에 대한 이유 : close(fd) 했을 때, kqueue는 소켓이 닫혔음을 내부적으로 자동으로 발견 가능
- 즉, close 전에, 관련 이벤트를 kqueue에서 수동 삭제할 필요 없음
[2023.03.20, 수정] EV_DELETE 잘못 알고 쓰고 있었음 -> 잘 알고 있었는데, Udata의 action 을 EVFILT_READ, EVFILT_WRITE 일 때 같이 사용해서 이상하게 사용됨

[-] EV_DELETE 제대로 쓰기
- WRITE 완료면 EV_DISABLE 해줘서 이벤트 감지 못하게 하기! -> 삭제..

Kimhan-nah

궁금한 점 올릴게용!

Kimhan-nah · 2023-03-19T04:04:27Z

src/controller/Executor.cpp

@@ -14,6 +14,18 @@ Executor::~Executor() {}

 Executor &Executor::operator=(const Executor &ref) { return (*this); }

+Client *Executor::creatClient(int fd) { return client_controller.insert(fd); }


creatClient -> createClient로 수정해야됩니당

Kimhan-nah · 2023-03-19T04:08:59Z

src/controller/Executor.cpp

+
+bool Executor::isConnected(Client *client) {
+    // TODO : SIGINT(CTRL-C)
+    return client && client->recv_buf.length();


연결이 끊기면 client->recv_buf에 EOF가 들어와서 length()가 0인 거죠?? 연결이 끊기지 않았는데 rec_buf.length()가 0인 경우는 없는 거죠??

handleRead에서 recv 한 내용이 어디 담기는지 확인해보시면 됩니다. 과제(subject)에 nc 로 SIGINT(CTRL-D) 사용하는거 같이 보시면 됩니당!

client의 연결이 끊겼는지(close 됐는지) kernel에서 자동으로 발견 가능하다고 했는데 그럼 length를 체크하는 방법 외에 다른 방법이 있는 거 아니에용??

커널단에서 알아 차려도 플래그로 표시해 주진 않습니다. man 페이지 다시 체크해보겠습니다.

Kimhan-nah · 2023-03-19T04:10:16Z

src/core/Server.cpp

@@ -86,7 +87,7 @@ void Server::handleAccept() {
    data->src = new_client;

    registerEvent(new_client->getFd(), CONNECT, data);
-    // registerEvent(new_client->getFd(), TIMEOUT, data); // TODO
+    _unregisters.insert(data);


connect 이벤트 등록한 이후에 왜 unregisters에 insert 하나요?? 연결이 된 경우에도 unregisters에 insert 하나요??

아 실제로 timeout된 client가 아니라 timeout 됐는지 check할 때 사용하고, 거기서 timeout인 경우 _garbage에 추가하는 거 맞나요??

1- 연결 된 경우, _unregisters 에서 erase 하는 부분을 작성하지 않았네요. 헷갈리게 해서 죄송합니다! 바로 수정하겠습니다!
2- 네 맞습니다! timeout 되면 _garbage 에 추가되서 삭제됩니다!

src/handler/EventHandler.cpp

Kimhan-nah · 2023-03-19T04:31:23Z

src/handler/EventHandler.cpp

            break;
        case DEL_WRITE:
-            EV_SET(&ev, fd, EVFILT_WRITE, EV_DELETE, 0, 0,
-                   static_cast<void *>(udata));
+            EV_SET(&ev, fd, EVFILT_WRITE, EV_DELETE, 0, 0, 0);


그럼 EV_DELETE 에서 EV_DISABLE??로 고치면 되는 거죠?

네 맞습니다~! 고치겠습니다~!

include/handler/EventHandler.hpp

nyeoni · 2023-03-19T04:49:33Z

src/core/Server.cpp

-        static_cast<Udata *>(_ev_list[event_idx].udata)->src->create_time;
-    // registerEvent(_ev_list[event_idx].ident, DEL_WRITE, 0);  // 나중에고치기
+void Server::handleTimeout() {
+    std::vector<Udata *> tmp; // iterator 생명주기..


코드를 보니까 tmp 를 안쓰고 구현해도 될 것 같습니다!
tmp 대신 _garbage 배열을 사용해서 _unregisters 구현하는 방향으로 리팩토링 하는건 어떨까용?
tmp 가 있어서 코드 가독성이 떨어지는 것 같아 제안드립니다 >_<

다음번에 해결하겠습니다 ㅠㅠ

nyeoni · 2023-03-19T04:53:12Z

src/core/Server.cpp

+    _garbage.clear();
+}
+
+bool Server::isConnected(Udata *udata) {


isConnected 가 executor 안에 있어야 하는 함수인지 잘 모르겠어요.
Server::isConnected 가 있다면 차라리 Executor::isConnected 를 없애고 다 server 안에서만 처리하는건 어떨까용?

Server::isConnected(Client *client) 로 하면 더 좋지 않을까 제안드립니다!

아 아니면 isConnected 가 아예 Client 멤버 함수로 있어도 좋을 것 같아요!

다음 번에 해결하겠습니다 !

chaejin-jen added the enhancement New feature or request label Mar 19, 2023

chaejin-jen requested review from Kimhan-nah and nyeoni March 19, 2023 01:00

chaejin-jen self-assigned this Mar 19, 2023

Kimhan-nah reviewed Mar 19, 2023

View reviewed changes

nyeoni requested changes Mar 19, 2023

View reviewed changes

✨ feat: Add EventHandler::garbageCollector

0b19783

chaejin-jen force-pushed the feature/close branch from 3402ded to 0b19783 Compare March 19, 2023 05:08

🐛 fix: conflict

ef989d6

nyeoni approved these changes Mar 19, 2023

View reviewed changes

Kimhan-nah merged commit 2623e8a into main Mar 19, 2023

chaejin-jen mentioned this pull request Mar 19, 2023

Close 관련 함수 리팩토링 #40

Closed

2 tasks

chaejin-jen deleted the feature/close branch March 23, 2023 04:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Add EventHandler::garbageCollector #39

[Feature] Add EventHandler::garbageCollector #39

chaejin-jen commented Mar 19, 2023 •

edited

Loading

Kimhan-nah left a comment

Kimhan-nah Mar 19, 2023

Kimhan-nah Mar 19, 2023

chaejin-jen Mar 19, 2023

Kimhan-nah Mar 19, 2023

chaejin-jen Mar 19, 2023

Kimhan-nah Mar 19, 2023

Kimhan-nah Mar 19, 2023

chaejin-jen Mar 19, 2023 •

edited

Loading

Kimhan-nah Mar 19, 2023

chaejin-jen Mar 19, 2023

nyeoni Mar 19, 2023

chaejin-jen Mar 19, 2023

nyeoni Mar 19, 2023

nyeoni Mar 19, 2023

nyeoni Mar 19, 2023

chaejin-jen Mar 19, 2023

		@@ -14,6 +14,18 @@ Executor::~Executor() {}

		Executor &Executor::operator=(const Executor &ref) { return (*this); }

		Client *Executor::creatClient(int fd) { return client_controller.insert(fd); }

[Feature] Add EventHandler::garbageCollector #39

[Feature] Add EventHandler::garbageCollector #39

Conversation

chaejin-jen commented Mar 19, 2023 • edited Loading

개요

작업내용

주의사항

Kimhan-nah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaejin-jen Mar 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaejin-jen commented Mar 19, 2023 •

edited

Loading

chaejin-jen Mar 19, 2023 •

edited

Loading