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

implement UserIdleTimeout #86

Closed
wants to merge 3 commits into from
Closed

Conversation

aojea
Copy link

@aojea aojea commented Jun 15, 2021

Implement a new method SetUserIdleTimeout() that allows to set a
timeout for idle sessions, but contrarty to the SetIdleTimeout(),
this doesn't take into account SPDY ping frames.

This allows consumers to use SPDY ping frames to keep the TCP and
SPDY connections alive, but also to detect and close the connection
if it is not being used, i.e. no data is sent by the applications
through the connection.

Signed-off-by: Antonio Ojea antonio.ojea.garcia@gmail.com

connection.go Outdated
@@ -158,6 +159,13 @@ func (i *idleAwareFramer) WriteFrame(frame spdy.Frame) error {
return err
}

if i.ignorePingFrames {
Copy link
Member

Choose a reason for hiding this comment

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

Accessing this value should still be accessed with a lock. Maybe switching the timeout lock to rw would work here.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not an expert but I always was recommended to not use RWMutex unless I have a big number of goroutines reading , some golang issues claim it is not very performant golang/go#17973
In this case we are protecting a variable that is not likely to change so often, so I think that a simple mutex will not make a big difference, unless the user keeps changing the idletimeout very often :)

Implement a new method SetUserIdleTimeout() that allows to set a
timeout for idle sessions, but contrarty to the SetIdleTimeout(),
this doesn't take into account SPDY ping frames.

This allows consumers to use SPDY ping frames to keep the TCP and
SPDY connections alive, but also to detect and close the connection
if it is not being used, i.e. no data is sent by the applications
through the connection.

Signed-off-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>
running the tests with -race flag shows that there is a race
trying to access the global variable "authenticated" during
the tests.

Using a lock to access the variable fixes it.

Signed-off-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>
Instead of using locking, that will cause read and write frames
to wait for each others and introduce a risk of deadlocking, use
atomic operations to check the new option to ignore ping SPDY
frames for idling.

Signed-off-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>
@aojea
Copy link
Author

aojea commented Jun 22, 2021

@dmcgowan I've implemented the locking using the atomic functions to avoid any possible issue with the other locks
83610fa

@adisky
Copy link

adisky commented Nov 11, 2021

@dmcgowan @aojea any update here?

@Joseph-Goergen
Copy link

@dmcgowan @aojea any update here?

@aojea
Copy link
Author

aojea commented Apr 8, 2022

I don't think this has any chances to merge, also, I honestly think that this parameter should be use to control session timeout ...

I'm going to close it to avoid confussions

@aojea aojea closed this Apr 8, 2022
@aojea aojea reopened this Jun 3, 2022
@aojea
Copy link
Author

aojea commented Jun 3, 2022

I closed it because of the lack of attention, but since may have some traction I'm going to reopen it

@haircommander
Copy link

the CRI-O community is still interested in this. Is there anything we can do to move this forward?

@dmcgowan
Copy link
Member

dmcgowan commented Nov 1, 2022

The change looks OK to me. Given the time its been since changes were made here, I think I would like to see where this change is used and how it is tested by library users to feel the most comfortable merging it.

@Joseph-Goergen
Copy link

Just checking to see where this PR is sitting

@BenTheElder
Copy link

containerd/containerd#5563 Would be the driving usage

@jrvaldes
Copy link

the CRI-O community is still interested in this. Is there anything we can do to move this forward?

@aojea ^

@aojea
Copy link
Author

aojea commented Feb 14, 2023

For people coming to this PR, this will be solved once this is implemented kubernetes/kubernetes#115493

This PR is a workaround to the underly problem that is that implementations are conflating USER session and TCP session, but with kubernetes/kubernetes#115493 people can opt-out of SPDY pings (and suffer the problems of not having them, ie. connections silently closed by intermediate devices , like NAT boxes that depend on traffic to renew the timers(

@aojea aojea closed this Feb 14, 2023
@jrvaldes
Copy link

I follow. Thanks @aojea for the explanation.

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.

7 participants