-
Notifications
You must be signed in to change notification settings - Fork 420
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
[GSoC 2021] Socket Library #17960
[GSoC 2021] Socket Library #17960
Conversation
b296385
to
8e5a1ba
Compare
3e8dc61
to
76c1bc8
Compare
6ffa98d
to
09c99be
Compare
c32978f
to
4df77fc
Compare
76532b3
to
b84457f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a review pass mostly over TCP part. Looks good. Made some inline comments.
@king-11 Looking forward for tests. For documentation, I believe you'll be writing doc-strings as comments over the functions. Also, please resolve merge conflicts.
f51b51b
to
eb16558
Compare
0b62c90
to
b9cf141
Compare
d4f4453
to
57778e8
Compare
a22d87a
to
f7e2bb0
Compare
245d1b1
to
7ef2c7f
Compare
modules/standard/Socket.chpl
Outdated
|
||
/* | ||
Send `data` over socket to the provided address and | ||
return number of bytes sent if successful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it is written now it looks like it might send only some bytes but not all of the passed bytes.
To send all of the passed bytes I think you would have to have a loop that tries again if only say 1 byte is sent by the sendto call at a time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can have another function like sendAll
which could do that because with udp we are working with packets instead of loads of data so if user wants to send large data they can use sendAll
. ref: This is how python does it https://docs.python.org/3/library/socket.html#socket.socket.send
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I'd support a sendAll
and also a comment here along the lines of this from those Python docs
Applications are responsible for checking that all data has been sent; if only some of the data was transmitted, the application needs to attempt delivery of the remaining data. For further information on this topic, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marked as future work
afb7eef
to
27023e2
Compare
keep functions on records at a single place together Signed-off-by: Lakshya Singh <lakshay.singh1108@gmail.com>
where clause and type checked generics for setSockOpt getSockOpt, getSockName Signed-off-by: Lakshya Singh <lakshay.singh1108@gmail.com>
@e-kayrakli @mppf changes were made as per review. |
We need to move the module from modules/standard/ to modules/packages and likewise the tests. We will need a test/packages/Socket.skipif file. See also https://chapel-lang.org/docs/main/developer/bestPractices/TestSystem.html#limiting-where-the-test-runs or look for examples in test/ with a name ending in .skipif. This skipif file needs to skip the testing:
I'm seeing C compilation failures on a system with libevent 2.0. We need to clearly document in the PR message and the module documentation the current caveats:
On another system where the tests compile, I'm seeing timeouts. @king-11 are the tests working for you? |
I looked at my notes in the PR and the recent commits, and they look good. Thanks @king-11! |
@mppf yes the tests are compiling and executing I think there can be issues with the |
@king-11 I don't think we should be connecting to google.com to run these tests. Instead, I think we should run a local Python webserver for the purposes of that test, or maybe remove it entirely. You can see an example of running our own webserver in test/library/packages/Curl/RunServer.chpl . |
I am still seeing timeouts. |
@mppf below are the updates:
|
I tried running on a Mac with Homebrew installed libevent. (To do that I had to change the paths in the require lines in Socket.chpl). I see many failures:
The testing I previously told you about failing connectblocking.chpl is using Ubuntu 21.10. It still seems to be timing out. Can you use some VMs or Docker images or something to try it on some different systems? While I believe it is working on your system it seems it does not work on other systems. (In fact, of 3 systems I've tried it on, I've seen problems in all 3). |
|
thanks @king-11. I think it's important to get all of the tests passing somewhere reproduce-able before we merge it. Can you look into the tcpio.chpl hang you are seeing? |
ace831f
to
d0ee4c1
Compare
Signed-off-by: Lakshya Singh <lakshay.singh1108@gmail.com>
Signed-off-by: Lakshya Singh <lakshay.singh1108@gmail.com>
some systems fail to resolve in cases with Unspec Signed-off-by: Lakshya Singh <lakshay.singh1108@gmail.com>
d0ee4c1
to
d02ec8a
Compare
check for ipv6 support if not there then dont test Signed-off-by: Lakshya Singh <lakshay.singh1108@gmail.com>
Signed-off-by: Lakshya Singh <lakshay.singh1108@gmail.com>
I've run testing on 2 systems with libevent 2.1.12 and all tests are passing and the skipifs seem to be working as expected. Great! I'm not going to merge today though because I'll be away the rest of the week. We'll consider including it in the 1.25.1 point release. |
Update copyright in Socket library Follow-up to PR #17960. Trivial and not reviewed.
This PR adds an implementation of the Socket library to modules/packages.
This first version has some caveats:
Design
#17899
Future Work