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

Add socket read timeout implementation #15261

Merged
merged 11 commits into from
May 13, 2019
Merged

Add socket read timeout implementation #15261

merged 11 commits into from
May 13, 2019

Conversation

wggihan
Copy link
Contributor

@wggihan wggihan commented May 8, 2019

Purpose

Current read actions doesn't support read timeout. It will hang until single byte receives from the other party. But this action needs to support a timeout.

Approach

Describe how you are implementing the solutions along with the design details.

Samples

service timeoutServer on new socket:Listener(61599, config = {readTimeout: 20000}) {

    resource function onConnect(socket:Caller caller) {       
    }

    resource function onReadReady(socket:Caller caller) {        
    }

    resource function onError(socket:Caller caller, error er) {        
    }
}

Remarks

Current default timeout value will be set as a 5 minutes. Timeout value needs to set in milliseconds.

Check List

  • Read the Contributing Guide
  • Required Balo version update
  • Updated Change Log
  • Checked Tooling Support (#)
  • Added necessary tests
    • Unit Tests
    • Spec Conformance Tests
    • Integration Tests
    • Ballerina By Example Tests
  • Increased Test Coverage
  • Added necessary documentation
    • API documentation
    • Module documentation in Module.md files
    • Ballerina By Examples

@codecov-io
Copy link

codecov-io commented May 9, 2019

Codecov Report

Merging #15261 into master will increase coverage by 0.12%.
The diff coverage is 88.67%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #15261      +/-   ##
============================================
+ Coverage     23.24%   23.36%   +0.12%     
- Complexity     5065     5071       +6     
============================================
  Files          1342     1341       -1     
  Lines         67464    67600     +136     
  Branches       9192     9200       +8     
============================================
+ Hits          15680    15798     +118     
- Misses        50046    50064      +18     
  Partials       1738     1738
Impacted Files Coverage Δ Complexity Δ
...nalang/stdlib/socket/endpoint/tcp/client/Read.java 100% <100%> (ø) 7 <0> (ø) ⬇️
...ng/stdlib/socket/endpoint/tcp/server/Register.java 100% <100%> (ø) 4 <0> (ø) ⬇️
...stdlib/socket/endpoint/udp/client/ReceiveFrom.java 77.77% <100%> (+4.44%) 5 <0> (ø) ⬇️
.../stdlib/socket/endpoint/tcp/server/InitServer.java 68% <100%> (+4.36%) 3 <0> (ø) ⬇️
...tdlib/socket/endpoint/udp/client/InitEndpoint.java 56.81% <100%> (+2.05%) 6 <0> (ø) ⬇️
...nalang/stdlib/socket/tcp/ReadPendingSocketMap.java 100% <100%> (ø) 6 <1> (ø) ⬇️
...rinalang/stdlib/socket/tcp/ReadReadySocketMap.java 100% <100%> (ø) 6 <1> (ø) ⬇️
...tdlib/socket/endpoint/tcp/client/InitEndpoint.java 68.96% <100%> (+2.29%) 3 <0> (ø) ⬇️
...ballerinalang/stdlib/socket/tcp/SocketService.java 76.92% <100%> (+4.19%) 4 <1> (+1) ⬆️
...llerinalang/stdlib/socket/tcp/SelectorManager.java 76.76% <100%> (+0.49%) 50 <0> (ø) ⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a521bb5...172dbdb. Read the comment docs.

@wggihan wggihan added this to the 0.995.0 milestone May 9, 2019
@wggihan wggihan added Team/StandardLibs All Ballerina standard libraries Type/Improvement labels May 9, 2019
@wggihan wggihan changed the title [WIP] Add socket read timeout implementation Add socket read timeout implementation May 9, 2019
socketChannel.write(buf);
}
Thread.sleep(2000L);
timeoutLeecher.waitForText(22000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having a thread sleep can't we only use the leecher timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The read will timeout after 20 seconds in here. Added thread.sleep add some grace period to prevent intermittent test failures when the server get busy due to some other factors.

stdlib/socket/src/main/ballerina/socket/client_socket.bal Outdated Show resolved Hide resolved
*/
public static long getReadTimeout() {
long timeout = DEFAULT_READ_TIMEOUT_VALUE;
if (ConfigRegistry.getInstance().contains(CONFIG_READ_TIMEOUT)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO reading config value as a runtime param should have the highest priority. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally closest config to the code should take the precedent. Also, this value affects globally.

log:printInfo("Client close: " + caller.remotePort);
}
} else {
io:println(result.detail().message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use log for this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LogLeecher matches exact words. When we used log it print timestamp as well.

@wggihan wggihan merged commit 9f4ea06 into ballerina-platform:master May 13, 2019
@wggihan wggihan deleted the timeout branch May 13, 2019 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team/StandardLibs All Ballerina standard libraries Type/Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants