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

[Enhancement-208] Add an option to send multiple times the same files #628

Closed
wants to merge 32 commits into from

Conversation

RCL98
Copy link
Contributor

@RCL98 RCL98 commented Nov 7, 2023

Q: What is the purpose of the enhancement?

  • To allow the same files/folders to be sent multiple times on the same session.

Q: What changed?

  • New flags for the send command have been added: multiple and timeout (see README.md changes for their purpose).
  • Rooms will now be kept alive for at most 60 minutes.
  • Rooms are now separated into main and secondary.
  • Main rooms = the room used for the initial connection and communications between sender and receivers
  • Secondary rooms = alternative rooms used for file transfers only (the ones opened after the main connection is established successfully).
  • Secondary rooms are deleted after first use.
  • Main rooms are kept alive by the TCP server until their maximum number of transfers is achieved (successful or not). This is set with the multiple flag. By default is 1.
  • If a receiver connects to the main room and the sender is not busy the transfer commences.
  • If a receiver connects the main room and the sender is busy with a transfer, the receiver will enter a queue. There is no limit to how many can enter the queue, just in case some receivers decide to exit early.
  • After a transfer is done, if the queue is not empty we extract the front receiver and begin a new transfer.
  • After the maximum number of transfers is done room is set for deletion. If there are still receivers in the waiting in the queue they will receive a signal to close the connection.
  • A sender will keep listening to the TCP server until its connection is severed by it. If a handshake is received a transfer commences, and the listening is paused until the transfer is finished.
  • Some refactoring was done on croc and tcp for easier understanding.
  • More unit tests added for tcp.

Q: Why sequential and not parallel transfers?

  • I'm not aware of how much load can the external relay server take, both from a hardware and a financial point of view :). So I decided this is the safer option.
  • This can potentially be extended to allow parallel transfers.
  • Sequential is good enough for the transfer of a reasonable number of small to medium files. If you want to transfer large or many files to multiple people in an efficient manner, you can use torrent.

@RCL98 RCL98 marked this pull request as draft November 7, 2023 22:39
@schollz
Copy link
Owner

schollz commented Nov 8, 2023

This is an amazing PR!!! Thank you so much for contributing it. I will take some time to digest it, hopefully get to it this weekend. Generally looks great, I like the approach and design. Please ping me if I don't come back in a week.

@schollz
Copy link
Owner

schollz commented Nov 9, 2023

Looks like it doesn't build yet:

src/cli/cli.go:307:97: not enough arguments in call to croc.GetFilesInfo
	have ([]string, bool)
	want ([]string, bool, bool)

@RCL98
Copy link
Contributor Author

RCL98 commented Nov 10, 2023

Hi @schollz, yeah I messed something up when I did the merge with main, I'll fix it right up.
In the meanwhile please wait to review until I get this PR out of draft, since I want to make some changes to my current implementation, which should fix all the problems with random test fails that are happening right now.
Thanks!

@RCL98 RCL98 marked this pull request as ready for review November 16, 2023 21:20
@RCL98
Copy link
Contributor Author

RCL98 commented Nov 16, 2023

Ready for review:)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@navaneeth-dev navaneeth-dev left a comment

Choose a reason for hiding this comment

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

Just found some minor spelling mistakes 😊

@schollz schollz closed this Feb 8, 2024
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.

3 participants