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

perf(backend): use websockets/ws instead of theturtle32/WebSocket-Node #10884

Merged
merged 14 commits into from
May 29, 2023
Merged

Conversation

syuilo
Copy link
Member

@syuilo syuilo commented May 24, 2023

Resolve #10883

What

Why

Additional info (optional)

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added the packages/backend Server side specific issue/PR label May 24, 2023
@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #10884 (57d1b2d) into develop (fd7b77c) will decrease coverage by 0.02%.
The diff coverage is 83.75%.

@@             Coverage Diff             @@
##           develop   #10884      +/-   ##
===========================================
- Coverage    77.34%   77.33%   -0.02%     
===========================================
  Files          740      740              
  Lines        70409    70428      +19     
  Branches      6423     6422       -1     
===========================================
+ Hits         54459    54465       +6     
- Misses       15950    15963      +13     
Impacted Files Coverage Δ
...ackend/src/server/api/StreamingApiServerService.ts 85.41% <80.59%> (-8.19%) ⬇️
packages/backend/src/server/ServerService.ts 48.90% <100.00%> (+0.22%) ⬆️
...ages/backend/src/server/api/AuthenticateService.ts 56.81% <100.00%> (ø)
packages/backend/src/server/api/stream/index.ts 75.08% <100.00%> (-0.09%) ⬇️

@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

Merging #10884 (6f8d921) into develop (1de774f) will increase coverage by 17.45%.

なぜ?

@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

Cannot destructure property 'user' of 'ctx' as it is undefined. なんでだ

@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

noServer: true にしないとダメかも

@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

ローカルでは動いた

@syuilo syuilo marked this pull request as ready for review May 24, 2023 02:51
@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

フロントエンドのテストが失敗しているのは関係なさそう

@github-actions github-actions bot requested a review from tamaina May 24, 2023 02:51
@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

バックエンドも落ちたわ

@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

これは関係あるかも

@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

connection.once('close' が呼ばれてなさそうな雰囲気

@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

@syuilo
Copy link
Member Author

syuilo commented May 24, 2023

何らかの理由でnestインスタンスがシャットダウンされてもstreaming処理が動いたままになってしまっていそうということは分かるけどなぜそうなるのかが不明
OnApplicationShutdownで閉じてはいるんだけども

@syuilo
Copy link
Member Author

syuilo commented May 25, 2023

ヌァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァンンンンオオオオンンオンオンオンオンンンンンンンンン゛ン゛!!!!!!!!!!!!!!!!

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

Ctrl+Cした時masterがkillされないみたいな感じよな

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

ローカルで動かすとメモリリークしまくる?

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

image

まずこれが謎

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

NoteはuploadUrlにめっちゃ時間がかかってるっぽい

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

uploadUrlってなんかストリーミングと関係あったっけと思ったら普通にあった

export const uploadUrl = async (user: any, url: string) => {
let file: any;
const marker = Math.random().toString();
const ws = await connectStream(user, 'main', (msg) => {
if (msg.type === 'urlUploadFinished' && msg.body.marker === marker) {
file = msg.body.file;
}
});
await api('drive/files/upload-from-url', {
url,
marker,
force: true,
}, user);

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

つまり

普通にストリーミングがバグってるだけだわ

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

'connect' with pong: trueしても'connected'が返ってこない

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

this.#wss.on('connection'はきてるけどtoken/appがnullになっとる

仕様だった

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

わかった

をawaitしている間に

ws.send(JSON.stringify({

が来てしまうため、connectできないということだった

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

image

手元ではテストが成功している

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

何らかの理由でnestインスタンスがシャットダウンされてもstreaming処理が動いたままになってしまっていそうということは分かるけどなぜそうなるのかが不明

これはredis周りの話なので関係ない感じがある

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

(redis周りというかprocess.exitが来てない感じがあり

@tamaina
Copy link
Contributor

tamaina commented May 28, 2023

workerしか殺していない(workerのdied :(しか見えない)

@syuilo
Copy link
Member Author

syuilo commented May 29, 2023

わかった

をawaitしている間に

ws.send(JSON.stringify({

が来てしまうため、connectできないということだった

this.userがあるならthis.userProfileなどもあるという前提で書かれているからfetchが完了する前にメッセージ受け付けるようにすると何か不具合が起きそう

@tamaina
Copy link
Contributor

tamaina commented May 29, 2023

fetchのPromiseを持っておいてawaitを必要な箇所に仕込めば良くねと思ったけど、なんか色々無理がありそう

バックエンド側が準備できたらreadyメッセージを送って、クライアントにはそれを待たせるとかがいいのか?

@syuilo
Copy link
Member Author

syuilo commented May 29, 2023

theturtle32/WebSocket-Node時代では普通にこれで動いてたけどwebsockets/wsで動かなくなった理由が分かっていない

@tamaina
Copy link
Contributor

tamaina commented May 29, 2023

server.on('upgrade', のコールバック関数内でawait main.init(connection);しなくなったから説

@syuilo
Copy link
Member Author

syuilo commented May 29, 2023

あー前まではthis.fetchが終わらないとwsコネクションの確立自体してないからか

@tamaina
Copy link
Contributor

tamaina commented May 29, 2023

this.#wss.emit('connection', の前にawait main.init(connection);する必要があるかも?

@syuilo
Copy link
Member Author

syuilo commented May 29, 2023

server.on('upgrade' 内でMainStreamConnectionの初期化しょりやれば解決するかも

@syuilo syuilo merged commit f930eae into develop May 29, 2023
@syuilo syuilo deleted the ws branch May 29, 2023 04:32
@syuilo
Copy link
Member Author

syuilo commented May 29, 2023

🙏🏻🙏🏻🙏🏻

sasagar pushed a commit to sasagar/misskey that referenced this pull request Jun 5, 2023
misskey-dev#10884)

* perf(backend): use websockets/ws instead of theturtle32/WebSocket-Node

Resolve misskey-dev#10883

* refactor

* Update StreamingApiServerService.ts

* Update StreamingApiServerService.ts

* ✌️

* Update StreamingApiServerService.ts

* fix main stream init

* fix timing 2

* setIntervalの重複を避ける(気休め)

* add comment

* ✌️

---------

Co-authored-by: tamaina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend Server side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

theturtle32/WebSocket-Node をやめる
2 participants