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

Workaround for actix-web bug. #379

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Workaround for actix-web bug. #379

merged 1 commit into from
Jul 19, 2023

Conversation

ryzhyk
Copy link
Contributor

@ryzhyk ryzhyk commented Jul 19, 2023

There is a bug in actix (actix/actix-web#1313) that prevents it from dropping HTTP connections on client disconnect unless the endpoint periodically sends some data. As a result, all API-based data transfers eventually start to fail in the UI. As a workaround, we generate an empty output chunk if there is no real payload to send for more than 3 seconds.

There is a bug in actix (actix/actix-web#1313)
that prevents it from dropping HTTP connections on client disconnect
unless the endpoint periodically sends some data.  As a result, all
API-based data transfers eventually start to fail in the UI.  As a
workaround, we generate an empty output chunk if there is no real
payload to send for more than 3 seconds.

Signed-off-by: Leonid Ryzhyk <leonid@feldera.com>
@ryzhyk ryzhyk requested a review from gz July 19, 2023 14:28
@Kixiron
Copy link
Contributor

Kixiron commented Jul 19, 2023

Wouldn’t it be better to add a ping/ack endpoint? That’d also allow us to check for uptime in addition to addressing this

@ryzhyk
Copy link
Contributor Author

ryzhyk commented Jul 19, 2023

Problem is, when the client disconnects there won't be a ping and the connection will remain around forever.

@Kixiron
Copy link
Contributor

Kixiron commented Jul 19, 2023

Well yah, when the client disconnects it can no longer send pings to the server so the connection gets dropped

@ryzhyk
Copy link
Contributor Author

ryzhyk commented Jul 19, 2023

Ok, fair enough. But today you can monitor a table with a simple curl command that returns a continuous stream of changes. Forcing the client to send keep-alive messages will make this harder to use.

// that prevents it from dropping HTTP connections on client disconnect
// unless the endpoint periodically sends some data. As a workaround,
// if there is not real payload to send for more than 3 seconds, we will
// generate an empty chunk. Note that it takes 6s, i.e., 2x the timeout
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this timeout period internal to actix or something we can (or do) configure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actix doesn't care. Basically, whenever a chunk is sent out, actix checks connection status and drops the request if the client closed their end of the connection.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Would we need to make this 3s duration configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super eager to add that control knob, and especially expose it to the client. Let's not do it now.

@ryzhyk ryzhyk merged commit 8dcf1a6 into main Jul 19, 2023
5 checks passed
@ryzhyk ryzhyk deleted the workaroun_actix_1313 branch July 19, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants