-
Notifications
You must be signed in to change notification settings - Fork 174
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
0.5 cleanup spec added to json tests #55
0.5 cleanup spec added to json tests #55
Conversation
- backward compatibility of exposed API preserved - symmetric implementation for server and client - fail-fast for server/client builders for WSS not initialized with SSLContext/SSLSocketFactory - More explanatory exception for scheme and configuration mismatch for client than just NPE
WssFactoryBuilder and WssSocketBuilder now expose only the minimum required API
# Conflicts: # ocpp-v1_6/src/main/java/eu/chargetime/ocpp/WebSocketTransmitter.java # ocpp-v1_6/src/main/java/eu/chargetime/ocpp/wss/BaseWssSocketBuilder.java
The CI terminates in a new and weird way for this PR:
|
Seems build hangs for some reason although nothing was added but cleanup (server side stop). I will think how to reproduce locally. |
WebSocketListener: logging bug fixed Fake system expose isClosed flag directly from server to check server state, when stopped isStarted flag reset, method added to clear riggedToFail flag Tests are refactored: setup(Spec) and cleanup(Spec) moved to the parent class, system (server side) do startup/shutdown cycles for each feature and there is delay to ensure startup/shutdown, riggedToFail flag is cleared as a part of cleanup
Codecov Report
@@ Coverage Diff @@
## master #55 +/- ##
============================================
- Coverage 51.5% 51.04% -0.46%
Complexity 759 759
============================================
Files 179 180 +1
Lines 3021 3048 +27
Branches 219 218 -1
============================================
Hits 1556 1556
- Misses 1380 1407 +27
Partials 85 85 |
…added_to_json_tests
… be enough for tests - later on the value might be changed to appropriate)
Logging added to fake central system for both starting and stopping WebSocketTransmitter and WebSocketListener updated to avoid NPE at stopping process Closing of Web Socket removed from WebSocketListener as WebSocketServer is taking care of it
I'm sure you have seen, but the IC says 14 failures and 6 errors now:
|
Also let's check for TIME_WAIT connections
…atures to set JSON server/client network configuration setPingInterval method removed as duplication of configuration parameter (can we do it as it was added just recently?) JSON server/client constructors updated to accept configuration For fakse central system configuration with reuse_addr flag added to prevent failures on travis ci Time delay before starting central system removed from base spec (let's check if reuse_addr suffice)
…atures to set JSON server/client network configuration setPingInterval method removed as duplication of configuration parameter (can we do it as it was added just recently?) JSON server/client constructors updated to accept configuration For fakse central system configuration with reuse_addr flag added to prevent failures on travis ci Time delay before starting central system removed from base spec (let's check if reuse_addr suffice)
@TVolden finally travis have done proper thing. Although I am not completely satisfied how it's done (still with some pauses via Thread.sleep before client connections) but still I think overall improved. I had to force implementation of the network configuration for JSON as it was needed to set reuse_addr due to specific of container where Travis CI runs (hanging connections in TIME_WAIT state). I've merged ping interval in configuration as well and removed from API. Do you think it's ok as it's minor thing and was merged very recently? |
@@ -132,6 +132,9 @@ public void onStart() { | |||
} | |||
|
|||
server.setConnectionLostTimeout(pingInterval); | |||
|
|||
server.setReuseAddr(true); |
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.
That's what might solve all 'not connected' errors.
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.
@sumlin for WebSocket default value of reuseAddr is false. Also according to the Socket specification initially this flag is not set. According to what I witnessed on two Windows and one Ubuntu machines tests are running fine without the flag set and only in Travis CI container we do have issues. Therefore I think not to set it by default but rather provide to end users more control over it by exposing json network configuration through API as discussed before.
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.
@eupakhomov well, sometimes if you stops your bind-address operation, Linux core doesn't release a port, it is not according WS, that's core network stuff. I have no idea why 'false' state is a default state, so I vote to set is as 'true' by default, I do this all my life without any problems. @TVolden what do you think?
@sumlin do you have also client-side related experience? If mentioned above
is truth for Android -based devices as well?
чт, 19 Апр 2018 г., 10:56 sumlin <[email protected]>:
… ***@***.**** commented on this pull request.
------------------------------
In ocpp-v1_6/src/main/java/eu/chargetime/ocpp/WebSocketListener.java
<#55 (comment)>
:
> @@ -132,6 +132,9 @@ public void onStart() {
}
server.setConnectionLostTimeout(pingInterval);
+
+ server.setReuseAddr(true);
@eupakhomov <https://github.com/eupakhomov> well, sometimes if you stops
your bind-address operation, Linux core doesn't release a port, it is not
according WS, that's core network stuff. I have no idea why 'false' state
is a default state, so I vote to set is as 'true' by default, I do this all
my life without any problems. @TVolden <https://github.com/TVolden> what
do you think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#55 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AOpUGGfrcGSpRU73Tmg6XUwbd6KCaOE-ks5tqFFFgaJpZM4TZIx2>
.
|
@eupakhomov a few projects. Unfortunately, I have no powerful experience with Android. You may read this manual http://man7.org/linux/man-pages/man7/socket.7.html SO_REUSEPORT part. Or this https://superuser.com/a/127865 . It seems if you kill your application and it doesn't close all sockets correctly, a new instance of your application may catch the packets that have been sent to the old application. But it is not important for us I guess. I believe @TVolden should decide do we use SO_REUSEPORT or not. |
Yeah let's just have it set to true by default, that sounds good. Can't wait to stabilize the intrgration tests, they kinda loose their value when they fail all the time. I'll review it tomorrow. Thanks for all your hard work, I really appreciate it. |
Looks like a small merge conflict with the imports |
Conflicts resolved Base test classes moved to the separate package New tests derived from base tests classes
No description provided.