implement UserIdleTimeout#86
Conversation
| return err | ||
| } | ||
|
|
||
| if i.ignorePingFrames { |
There was a problem hiding this comment.
Accessing this value should still be accessed with a lock. Maybe switching the timeout lock to rw would work here.
There was a problem hiding this comment.
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>
38360bc to
ee4c911
Compare
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>
ee4c911 to
83610fa
Compare
|
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 |
|
I closed it because of the lack of attention, but since may have some traction I'm going to reopen it |
|
the CRI-O community is still interested in this. Is there anything we can do to move this forward? |
|
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. |
|
Just checking to see where this PR is sitting |
|
containerd/containerd#5563 Would be the driving usage |
@aojea ^ |
|
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( |
|
I follow. Thanks @aojea for the explanation. |
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