-
Notifications
You must be signed in to change notification settings - Fork 3
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
Bug/#315 unsubscribe, subscribe에 queue 적용 #337
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다 👍 👍
|
||
await this.mutex.runExclusive(async () => { | ||
const connectedSockets = await this.server.to(stockId).fetchSockets(); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try-catch로 묶으신 이유가 궁금합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 try-catch로 에러처리하는 부분이 따로 없고, 외부 유저와 connection하는 부분이다 보니 예기치 못한 에러가 발생할 수 있다고 생각했습니다
@@ -7,24 +6,23 @@ import { RawData, WebSocket } from 'ws'; | |||
export class WebsocketClient { | |||
static url = process.env.WS_URL ?? 'ws://ops.koreainvestment.com:21000'; | |||
private client: WebSocket; | |||
//현재 factory 패턴을 이용해 할당하면 socket이 열리기 전에 message가 가는 문제가 있음. | |||
// 소켓이 할당되기 전에(client에 소켓이 없을 때) message를 보내려 시도함. | |||
private messageQueue: string[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shift하면 비용이 n이라 비효율적이라 큐를 쓰는게 좋긴하지만, JS에서는 직접 구현해야되는게 아쉽네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 너무 아쉽습니다. 큐 말고 더 좋은 방법이 있을까 해서 onModuleInit도 고려했는 데, websocket을 열 수 있는 개수를 런타임때야 알 수 있으니 어쩔 수 없었다고 생각합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ~~
close #336
✅ 작업 내용
😎 체크 사항