WebSocket proxy for the grpc-gateway by hariso · Pull Request #752 · ConduitIO/conduit

hariso · 2022-11-28T11:46:45Z

Description

This PR extracts from https://github.com/tmc/grpc-websocket-proxy/ the most important parts of code to us. This library has been introduced as a dependency in #718.

Update: gorilla/websocket got archived: https://github.com/gorilla#gorilla-toolkit

Quick checks:

I have followed the Code Guidelines.
There is no other pull request for the same update/change.
I have written unit tests.
I have made sure that the PR is of reasonable size and can be easily reviewed.

commit 541cd83 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 29 14:32:49 2022 +0100 simplify tests commit 6737ab4 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 29 13:57:14 2022 +0100 cosmetics commit 9dbc8b1 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 29 13:36:32 2022 +0100 rename commit f218ed2 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 29 13:18:46 2022 +0100 tests commit b7f2baf Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 29 11:40:25 2022 +0100 simplify commit fd699c1 Author: Haris Osmanagic <haris@meroxa.io> Date: Mon Nov 28 13:02:55 2022 +0100 improvements commit 27bf6ec Author: Haris Osmanagic <haris@meroxa.io> Date: Mon Nov 28 12:47:54 2022 +0100 rename method commit 7ec409d Author: Haris Osmanagic <haris@meroxa.io> Date: Mon Nov 28 12:09:34 2022 +0100 linter commit 3593282 Author: Haris Osmanagic <haris@meroxa.io> Date: Mon Nov 28 12:02:16 2022 +0100 init copy commit 52ffbc7 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 22 20:24:40 2022 +0100 setup commit 623ca25 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 22 12:10:43 2022 +0100 pr feedback 3 commit 83c6152 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 22 11:34:29 2022 +0100 Revert "Update pkg/web/api/connector_v1_test.go" This reverts commit 8f40c22. commit 6df36fc Merge: dd8b1b4 8f40c22 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 22 11:29:21 2022 +0100 Merge branch 'haris/stream-inspector-destination' of github.com:ConduitIO/conduit into haris/stream-inspector-destination commit dd8b1b4 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 22 11:28:27 2022 +0100 linter commit 7865b60 Author: Haris Osmanagic <haris@meroxa.io> Date: Tue Nov 22 11:26:07 2022 +0100 pr feedback 2 commit 8f40c22 Author: Haris Osmanagić <haris@meroxa.io> Date: Tue Nov 22 11:18:58 2022 +0100 Update pkg/web/api/connector_v1_test.go Co-authored-by: Lovro Mažgon <lovro.mazgon@gmail.com> commit 5067241 Author: Haris Osmanagić <haris@meroxa.io> Date: Tue Nov 22 11:08:57 2022 +0100 Update pkg/inspector/inspector_test.go Co-authored-by: Lovro Mažgon <lovro.mazgon@gmail.com> commit e7b3306 Author: Haris Osmanagić <haris@meroxa.io> Date: Tue Nov 22 11:08:17 2022 +0100 Update pkg/inspector/inspector_test.go Co-authored-by: Lovro Mažgon <lovro.mazgon@gmail.com> commit 3a0b31b Author: Haris Osmanagić <haris@meroxa.io> Date: Tue Nov 22 10:58:40 2022 +0100 Update pkg/inspector/inspector_test.go Co-authored-by: Lovro Mažgon <lovro.mazgon@gmail.com> commit 1a94458 Author: Haris Osmanagic <haris@meroxa.io> Date: Mon Nov 21 14:06:27 2022 +0100 linter commit ae1408f Author: Haris Osmanagic <haris@meroxa.io> Date: Mon Nov 21 14:05:46 2022 +0100 linter commit 1848a05 Author: Haris Osmanagic <haris@meroxa.io> Date: Mon Nov 21 14:05:01 2022 +0100 Stream inspector for destinations

lovromazgon

Seems like we will still need a read loop to figure out when the client closed the connection so we can cancel our context (this and this). Right now Conduit won't stop even if the client stops the inspector session because it thinks the connection is still open.

pkg/foundation/grpcutil/websocket.go

hariso · 2022-12-19T12:12:33Z

Seems like we will still need a read loop to figure out when the client closed the connection so we can cancel our context (this and this). Right now Conduit won't stop even if the client stops the inspector session because it thinks the connection is still open.

Can you tell me a bit more about how you tested it? I tested this a few times in the following way:

Start Conduit with a pipeline
Inspect a session with wscat. Wait for some records.
Kill wscat
Stop Conduit with Ctrl-+C or kill -SIGINT

This worked. There's an issue with shutting down with an open inspector session though (#747).

lovromazgon · 2022-12-19T13:24:51Z

Here are steps to reproduce what I mean:

Terminal 1: ./conduit -pipelines.path=examples/pipelines (provisions the example pipeline)
Terminal 2: wscat -c ws://localhost:8080/v1/connectors/file-to-file:example.out/inspect
Terminal 3: echo "hi" >> example.in (see the record appear in the inspector and in example.out)
Terminal 2: Ctrl+C (kill wscat)
Terminal 1: Ctrl+C (kill Conduit)

Conduit then doesn't shut down, also the message session removed is never logged.

Note that you can reproduce the same thing by simply navigating to http://localhost:8080/ui/pipelines/file-to-file, since the UI opens an inspector session. Even after closing the tab Conduit thinks the session is still open.

hariso · 2022-12-19T13:42:05Z

@lovromazgon Thanks for that! I used the same steps, but after I tried it out a few times I realized it takes a bit of time for the context to be closed and the session to be removed. In other words, if you perform steps 4 and 5, you'll get to the behavior you described. But if there's a short pause, then it's all good. Thanks for noticing this, I'll look into what's causing it.

commit 5dad631 Author: Haris Osmanagic <haris@meroxa.io> Date: Thu Dec 22 13:34:34 2022 +0100 linter commit 3c9fef2 Author: Haris Osmanagic <haris@meroxa.io> Date: Thu Dec 22 13:07:45 2022 +0100 ctx check commit 7368327 Author: Haris Osmanagic <haris@meroxa.io> Date: Thu Dec 22 11:55:49 2022 +0100 ping pongs working commit 088852f Author: Haris Osmanagic <haris@meroxa.io> Date: Wed Dec 21 14:55:49 2022 +0100 Revert "remove ping-pong" This reverts commit b584fcf.

pkg/foundation/grpcutil/websocket_test.go

lovromazgon

Nice, good work!

pkg/foundation/grpcutil/websocket.go

Co-authored-by: Lovro Mažgon <lovro.mazgon@gmail.com>

hariso changed the base branch from main to haris/stream-inspector-destination November 28, 2022 11:46

lovromazgon mentioned this pull request Nov 29, 2022

Stream inspector for destinations #718

Merged

4 tasks

Base automatically changed from haris/stream-inspector-destination to main November 30, 2022 13:29

hariso force-pushed the haris/grpc-gateway-websockets branch from 541cd83 to 7cd3254 Compare November 30, 2022 13:38

hariso added 2 commits November 30, 2022 15:19

send scanner error

b51b9d0

send scanner error

596753a

hariso marked this pull request as ready for review November 30, 2022 14:31

hariso requested a review from a team as a code owner November 30, 2022 14:31

lovromazgon reviewed Dec 8, 2022

View reviewed changes

pkg/foundation/grpcutil/websocket.go Outdated Show resolved Hide resolved

pkg/foundation/grpcutil/websocket.go Show resolved Hide resolved

Merge branch 'main' into haris/grpc-gateway-websockets

9895167

rename

80c504d

hariso added 11 commits December 21, 2022 11:53

read loop

692a988

refactor + ping write loop

e3a87a1

refactor

2072f7d

fix pingWriteLoop

a16ca05

fixes

2ae9538

comments

eb7e986

remove ping-pong

b584fcf

Merge branch 'main' into haris/grpc-gateway-websockets

b030581

remove tests

b55a906

refactor

47f6221

hariso commented Dec 22, 2022

View reviewed changes

pkg/foundation/grpcutil/websocket_test.go Show resolved Hide resolved

lovromazgon and others added 3 commits December 23, 2022 14:37

fix concurrency

b0ff958

comments

7d4b410

test

9fac27d

hariso added 2 commits December 23, 2022 16:54

tests

b5da901

lint

2959aff

lovromazgon approved these changes Dec 23, 2022

View reviewed changes

pkg/foundation/grpcutil/websocket.go Outdated Show resolved Hide resolved

pkg/foundation/grpcutil/websocket.go Outdated Show resolved Hide resolved

hariso and others added 2 commits December 23, 2022 20:15

Update pkg/foundation/grpcutil/websocket.go

b8f1110

Co-authored-by: Lovro Mažgon <lovro.mazgon@gmail.com>

Update pkg/foundation/grpcutil/websocket.go

2601ad8

Co-authored-by: Lovro Mažgon <lovro.mazgon@gmail.com>

hariso enabled auto-merge (squash) January 2, 2023 11:11

hariso merged commit d13761f into main Jan 2, 2023

hariso deleted the haris/grpc-gateway-websockets branch January 2, 2023 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebSocket proxy for the grpc-gateway#752

WebSocket proxy for the grpc-gateway#752
hariso merged 23 commits intomainfrom
haris/grpc-gateway-websockets

hariso commented Nov 28, 2022 •

edited

Loading

Uh oh!

lovromazgon left a comment

Uh oh!

Uh oh!

Uh oh!

hariso commented Dec 19, 2022

Uh oh!

lovromazgon commented Dec 19, 2022

Uh oh!

hariso commented Dec 19, 2022

Uh oh!

Uh oh!

lovromazgon left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hariso commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Quick checks:

Uh oh!

lovromazgon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hariso commented Dec 19, 2022

Uh oh!

lovromazgon commented Dec 19, 2022

Uh oh!

hariso commented Dec 19, 2022

Uh oh!

Uh oh!

lovromazgon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hariso commented Nov 28, 2022 •

edited

Loading