-
Notifications
You must be signed in to change notification settings - Fork 116
step1 for fixing tcp connections delays, based on the discussion #3338 #3345
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
Conversation
|
That looks good except for re-using Is used on established TCP connections to verify if they're still alive. For this new connection timeout, we want a much shorter delay of 2..5 seconds. |
How about:
it is unfortunate indeed |
abrahamwolk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the option
org.phoebus.pv.pva/epics_pva_tcp_socket_tmo=2
in my phoebus.ini file does not have an effect. The reason is that a line needs to be added to the static initializer block of the class PVASettings:
diff --git a/core/pva/src/main/java/org/epics/pva/PVASettings.java b/core/pva/src/main/java/org/epics/pva/PVASettings.java
index eda3b03bc..4b2fd574e 100644
--- a/core/pva/src/main/java/org/epics/pva/PVASettings.java
+++ b/core/pva/src/main/java/org/epics/pva/PVASettings.java
@@ -279,6 +279,7 @@ public class PVASettings
EPICS_PVA_FAST_BEACON_MAX = get("EPICS_PVA_FAST_BEACON_MAX", EPICS_PVA_FAST_BEACON_MAX);
EPICS_PVA_MAX_BEACON_AGE = get("EPICS_PVA_MAX_BEACON_AGE", EPICS_PVA_MAX_BEACON_AGE);
EPICS_PVA_ENABLE_IPV6 = get("EPICS_PVA_ENABLE_IPV6", EPICS_PVA_ENABLE_IPV6);
+ EPICS_PVA_TCP_SOCKET_TMO = get("EPICS_PVA_TCP_SOCKET_TMO", EPICS_PVA_TCP_SOCKET_TMO);
}
/** Get setting from property, environment or default
I have now tested it, using the following setup:
What I observe is the following:
Given that I have set |
|
I think there's still the threading issue. When a new channel is requested, it's scheduled to be searched now, then in a second, then in two seconds, then in 4 seconds, ..., in 30 seconds. bucket 0 (now): Search channels A, B, C, D, E So one search message will be issued for channels A, B, C, D, E. With high log level, you should see the bucket intervals, the searches sent out for each bucket, the replies etc: |
|
@abrahamwolk I have only addressed the comments... I agree that we still have a threading issue. |
|
OK, merge this? |
Absolutely. This pull-request is certainly an improvement and a feature that is good to have. I didn't mean to imply that we shouldn't merge this, but rather provide a data-point on how the performance of the establishment of TCP connections is. Thanks for implementing this! |
I have used an existing setting/preference
I had to change to code to first create a unconnected socket and then try connect with a timeout. Previously we were using constructors which both created and connected to the address... but these constructors did not support configuring a timeout.
A @kasemir through review would be appreciated. @abrahamwolk can you test this branch with a reduced timeout to see what impact it has on your Phoebus behaviour.