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

fix(dgw,jetsocat): implement WebSocket keep-alive logic #1202

Merged
merged 5 commits into from
Jan 29, 2025

Conversation

CBenoit
Copy link
Member

@CBenoit CBenoit commented Jan 29, 2025

Our WebSockets are already responding Pong messages to Ping messages, but they were never sending Ping messages.

@CBenoit CBenoit enabled auto-merge (squash) January 29, 2025 00:56
T: fmt::Debug,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Debug::fmt(&self.0, f)
Copy link
Contributor

@pacmancoder pacmancoder Jan 29, 2025

Choose a reason for hiding this comment

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

Doesn't #[derive(Debug)] produce the same output?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not derivable because T does not necessarily implement Debug. Unless they improved that recently?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@CBenoit CBenoit Jan 29, 2025

Choose a reason for hiding this comment

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

Oh! Good to know!

However, I still prefer the current implementation which gives:

Mutex { data: 42 }

Instead of

PinnableMutex(Mutex { data: 42 })

The PinnableMutex part is not relevant, the same way it’s not relevant for Pin:
https://doc.rust-lang.org/nightly/src/core/pin.rs.html#1701-1705

Both Pin and PinnableMutex are irrelevant at runtime, it’s just type gymnastic to ensure code correctness at compile-time.

As a result, for this code:

use core::fmt;

use parking_lot; // 0.12.3

pub struct PinnableMutex<T: ?Sized>(parking_lot::Mutex<T>);

impl<T: ?Sized> fmt::Debug for PinnableMutex<T>
where
    T: fmt::Debug,
{
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        fmt::Debug::fmt(&self.0, f)
    }
}

fn main() {
    let pinned_mutex = core::pin::pin!(PinnableMutex(parking_lot::Mutex::new(42)));
    println!("{pinned_mutex:?}");
}

We get this output:

Mutex { data: 42 }

The only runtime-relevant information.

type Error = E;

fn poll_ready(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
self.inner.as_ref().lock().as_mut().poll_ready(cx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Blocking on sync mutex in async future feels wrong, does using async mutex reduce performance drastically?

Copy link
Member Author

@CBenoit CBenoit Jan 29, 2025

Choose a reason for hiding this comment

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

It does reduce performance considerably to use async Mutex, and it's not wrong to use a sync one unless you hold the mutex across async points. Here, it's not the case, the poll functions should return ASAP, either returning the result, or registering the task for later wake up using the Context/Waker.
I recommend this read: https://ryhl.io/blog/async-what-is-blocking/
And this one: https://tokio.rs/tokio/tutorial/shared-state

I used parking_lot mutex because it is the most lightweight and performant option for low-contention scenarios like this one. The performance hit is negligible.

Side merit: simpler code, no nested polling. (Of course, it's okay only thanks to the points above.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! Interesting, I'll take a read

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a commit to improve the documentation directly in the code: 9b42424

@pacmancoder pacmancoder self-requested a review January 29, 2025 10:05
@pacmancoder pacmancoder disabled auto-merge January 29, 2025 10:07
Copy link
Contributor

@pacmancoder pacmancoder left a comment

Choose a reason for hiding this comment

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

Looks good! Approved in advance, however I am a bit confused about parking_lot sync Mutex usage in futures, could you elaborate on this approach?

@CBenoit
Copy link
Member Author

CBenoit commented Jan 29, 2025

Looks good! Approved in advance, however I am a bit confused about parking_lot sync Mutex usage in futures, could you elaborate on this approach?

Of course! I should include the rationale in the documentation: #1202 (comment)

@CBenoit CBenoit enabled auto-merge (squash) January 29, 2025 14:05
@CBenoit CBenoit disabled auto-merge January 29, 2025 14:07
@CBenoit
Copy link
Member Author

CBenoit commented Jan 29, 2025

Also implemented the keepalive logic for Gateway -> Target server in /jet/fwd/tls route: 17f0a95

@CBenoit CBenoit enabled auto-merge (squash) January 29, 2025 14:18
@CBenoit CBenoit merged commit 22e9e7e into master Jan 29, 2025
37 checks passed
@CBenoit CBenoit deleted the fix/DGW-241-keep-alive branch January 29, 2025 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants