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.

Sign up for GitHub

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

Jump to bottom

Function disable_nagle moved to Sockets and documented #31924

Merged
merged 1 commit into from
Jun 6, 2019
Merged

Function disable_nagle moved to Sockets and documented #31924

merged 1 commit into from
Jun 6, 2019

Conversation

mateuszbaran
Copy link
Contributor

I've moved the diable_nagle functions from Distributed to Sockets, as suggested in #31842 . I was trying to make the PR small, so:

  1. disable_nagle precompile statements should probably go to the Sockets stdlib.
  2. I was thinking about adding some tests but libuv doesn't provide getsockopt to test that an option is actually set.

@jpsamaroo
Copy link
Member

Nitpick: Spelling of "disable_nagle" in commit message

Also, probably should have a NEWS entry?

@mateuszbaran mateuszbaran changed the title Function diable_nagle moved to Sockets and documented Function disable_nagle moved to Sockets and documented May 5, 2019
@StefanKarpinski
Copy link
Member

How about changing the API to nagle(::Bool) instead whole we’re at it?

@mateuszbaran mateuszbaran force-pushed the mbaran/disable-nagle branch from c596af2 to 9b71e13 Compare May 5, 2019 16:13
@mateuszbaran
Copy link
Contributor Author

@jpsamaroo thanks, I've fixed the commit message (and the PR title). I haven't exported disable_nagle so it probably doesn't need a NEWS entry.

If we are going to change the API, then maybe we should stay closer to the naming from libuv? For example tcp_nodelay(socket::Union{TCPServer, TCPSocket}, enable::Bool).
BTW, I've researched the TCP_QUICKACK flag that is also set in disable_nagle and it doesn't seem appropriate to bundle these two in a single function. TCP_NODELAY is set permanently for a given socket while TCP_QUICKACK is apparently reset by certain operations ( nodejs/node#21104 (comment) ).

@JeffBezanson
Copy link
Member

Ok, two functions sounds fine. tcp_nodelay and tcp_quickack? (Although I hate the double negative in nodelay=false...)

@mateuszbaran
Copy link
Contributor Author

OK, if you don't like tcp_nodelay then I'll name it nagle (and the other one will be tcp_quickack).

@mateuszbaran
disable_nagle moved to Sockets, split and documented (see JuliaLang#3…
ecde2ab 
…1842 )

disable_nagle was split into nagle (which enables or disables Nagle's 
algorithm) and quickack (which enables or disables TCP_QUICKACK on Linux 
systems).
@mateuszbaran mateuszbaran force-pushed the mbaran/disable-nagle branch from 9b71e13 to ecde2ab Compare May 24, 2019 09:33
@mateuszbaran
Copy link
Contributor Author

I've split disable_nagle to nagle and quickack and added enable arguments. Is this OK?

@StefanKarpinski
Copy link
Member

I'm pretty happy with this now but it does make me wonder if we shouldn't expose these as properties of the socket, i.e. sock.nagle and sock.quickack which can be read or assigned. @vtjnash or @Keno, do you have strong feelings about that?

@JeffBezanson JeffBezanson merged commit 7b59c72 into JuliaLang:master Jun 6, 2019
vtjnash
vtjnash reviewed Jun 6, 2019
View reviewed changes
stdlib/Sockets/src/Sockets.jl
"""
function nagle(sock::Union{TCPServer, TCPSocket}, enable::Bool)
# disable or enable Nagle's algorithm on all OSes
ccall(:uv_tcp_nodelay, Cint, (Ptr{Cvoid}, Cint), sock.handle, Cint(!enable))
Copy link
Member

Choose a reason for hiding this comment

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

Error checking of return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll make a separate PR for this.

@mateuszbaran mateuszbaran mentioned this pull request Jun 11, 2019
Open
Keno pushed a commit that referenced this pull request Jun 5, 2024
@mateuszbaran @JeffBezanson
disable_nagle moved to Sockets, split and documented (see #31842 ) (#…
7dd63a7 
…31924)

disable_nagle was split into nagle (which enables or disables Nagle's 
algorithm) and quickack (which enables or disables TCP_QUICKACK on Linux 
systems).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@JeffBezanson JeffBezanson JeffBezanson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mateuszbaran @jpsamaroo @StefanKarpinski @JeffBezanson @vtjnash