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

Extend reading with delay and retry. #201

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mbarnach
Copy link
Member

@mbarnach mbarnach commented Apr 1, 2021

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:

  • I have submitted a CLA form
  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

@sonarcloud
Copy link

sonarcloud bot commented Apr 1, 2021

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
Copy link
Contributor

dannys42 commented Apr 3, 2021

@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:

  • have a loop that contains a Socket.wait()
  • do a readString()
  • append the result to a buffer
  • scan the buffer for newlines.
  • If a newline exists, pull out the command and process it, then remove everything up to the newline from the buffer (leaving the rest there), then continue the loop
  • If a newline does not exist, do nothing and continue the loop

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
Copy link
Member Author

mbarnach commented Apr 6, 2021

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants