Kitura / BlueSocket

Socket framework for Swift using the Swift Package Manager. Works on iOS, macOS, and Linux.
Apache License 2.0
1.41k stars 197 forks source link

Extend reading with delay and retry. #201

Open mbarnach opened 3 years ago

mbarnach commented 3 years ago

Add a retry option with delay for reading through a socket. Issue: #176 Idea from @Nathipha

Description

The protocol for reading through a socket has been extended to handle a potential retry operation, with delay. This should allow slow server to respond on time.

Motivation and Context

This should fix the issue #176

How Has This Been Tested?

No real tests done, but existing behavior shouldn't change. @nathipha could you give it a try with in your context?

Checklist:

sonarcloud[bot] commented 3 years ago

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

dannys42 commented 3 years ago

@mbarnach thanks for putting up this PR... it helps to understand the problem. Looking at the BlueSocket APIs, I believe the intent is to be a lightweight swift wrapper around typical socket() calls. So I'm not sure if this is the right way to go about this.

Typically you would select() for activity on the sockets (embedded in the wait() call in BlueSocket), however the amount of data obtained from sockets are never guaranteed, except in the case of a blocking call (assuming the connection does not close while waiting of course).

The current implementation of readString() has no length parameter so I'm not sure what it will do in the case of a non-blocking socket. (I believe it will either it will return what's available in the kernel buffer or it will return the full buffer size requested). You could in theory add a length parameter to readString() (as you did), but since the original requestor is attempting to implement an FTP service, the commands possible are variable length. So it still wouldn't be useful in this case. Typically what I've had to do in the past is create a circular buffer where I can test for commands and remove them from the buffer when they match so as not to lose any remaining data.

Until we have a more general solution, for the FTP use-case, I think the way that @Nathipha should structure their code is to do something like this:

It's a little bit of work, but it's typical of what I've had to do when working with sockets at a low level.

Maybe we can consider adding a utility function to make this simpler for String or Data-based protocols.. ideally supporting non-blocking file descriptors allowing for better performance. I'm open to ideas, but I've never really seen much better when it comes to handling streaming protocols (especially ones with variable length commands.)

mbarnach commented 3 years ago

@dannys42 When doing the PR, I was under the impression it wasn't the right approach. First reason being the difficulty to test such behaviour (tests have not been committed as they are not fully functional). The intended behaviour could be achieved by an external implementation, as you have describe it. I would also rather not merge that PR. Maybe the better solution would be for you to formalised (in pseudo-code) your approach in the README or somewhere in the doc? Therefore, we will have a place to point people to it?