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

Fix issues #222 and #535. Enlarge the maximum size of UDP packet. #536

Merged
merged 6 commits into from
Mar 27, 2017

Conversation

Noskthing
Copy link
Contributor

Hi!

This branch fix issues #222( Don't trust GCD to give accurate UDP packet sizes) and #535(GCDAsyncUDPSocket can not send data when data is greater than 9K). The reason is that the default maximum size of the UDP buffer in iOS is 9216 bytes. So they can not send data if the size of data is grater than 9216 or packets can be silently truncated without any log.

I enlarge the maximum size of UDP packet by function setsockopt and change the value of max4ReceiveSize/max6ReceiveSize.

I test my changes and it works fine!

@chrisballinger
Copy link
Collaborator

@Noskthing Thanks for looking into this. What happens if the data you are sending is greater than 65535, will that silently drop without error? If so, perhaps an error should be thrown somewhere, and this behavior documented in the .h file.

Before I merge this would you mind adding unit tests?

  1. Create new UDPTests in Shared: https://github.com/robbiehanson/CocoaAsyncSocket/tree/master/Tests/Shared
  2. Add UDPTests to iOS, macOS, and Framework projects to ensure coverage for CocoaPods and direct Framework integration

You can look at the other TCP tests for an example. You can make a UDP serverSocket and clientSocket during setup, and then add tests for these cases:

  1. Sending/receiving packet < 9216
  2. Sending/receiving packet > 9216
  3. Sending/receiving packet > 65535

Copy link
Collaborator

@chrisballinger chrisballinger left a comment

Choose a reason for hiding this comment

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

Please add unit tests as described here: #536 (comment)

@Noskthing
Copy link
Contributor Author

Hi,

I am sorry that if the data you are sending is greater than limit the socket will be closed but i neglect this error. Delegate will excute function udpSocketDidClose:withError: .

I create new UDPTests in Shared but i don't know how to add to other projects. It just work in iOS project.

I think udp is not a good idea when you try to send a large packet so that apple set the maximum size of udp buffer 9216 not 65535. I add some code annotation and wish it could help others.

Copy link
Collaborator

@chrisballinger chrisballinger left a comment

Choose a reason for hiding this comment

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

The tests look great, thank you! I can add them to the other targets for you when I merge.

Only a few small changes left, looks like uint32_t was used instead of uint16_t.

* You must set it before the sockt is created otherwise it won't work.
*
**/
- (uint32_t)maxSendBufferSize;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be uint16_t not uint32_t correct?

- (uint32_t)maxSendBufferSize
{
__block uint32_t result = 0;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also should be uint16_t here as well?

@chrisballinger
Copy link
Collaborator

@Noskthing Also would you mind adding an additional test for the maxSendBufferSize setter/getter feature?

After those small changes I will merge because otherwise it looks good. 👍

@Noskthing
Copy link
Contributor Author

Thanks for your correction!

I have fixed these error and add tests for maxSendBufferSize. :)

@chrisballinger
Copy link
Collaborator

Awesome! Thank you

@chrisballinger chrisballinger merged commit 53a1298 into robbiehanson:master Mar 27, 2017
balord added a commit to Figure53/F53OSC that referenced this pull request Jan 3, 2018
Fixes compiler warnings and data type errors, plus these specific fixes:
* File descriptor leak - robbiehanson/CocoaAsyncSocket#522
* Fixes UDP connection issues with IPv6: robbiehanson/CocoaAsyncSocket#429 (comment)
* Prevents truncating UDP packets: robbiehanson/CocoaAsyncSocket#222
* Allow UDP packet size above iOS default of 9216 bytes: robbiehanson/CocoaAsyncSocket#536

NOTE: We are holding back on updating to GCDAsyncSocket 7.6.2 because of reports that 7.6.1 may have introduced a crash:
* see robbiehanson/CocoaAsyncSocket#579
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