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

add kernel-level distribution support(SO_REUSEPORT) for linux #1379

Merged
merged 2 commits into from Jan 29, 2016

Conversation

Projects
None yet
3 participants
@soarqin
Contributor

soarqin commented Jan 21, 2016

libasync does not have setOption for AsyncTCPListener so I do think it is impossible to support TCPListenOptions.reusePort in libasync driver at present.
Therefore I only add it to libevent/libev drivers.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jan 21, 2016

Contributor

It's only fair that it be enabled by default, I don't see any web server going without it. Which is why I re-enabled it by default in libasync directly.

Contributor

etcimon commented Jan 21, 2016

It's only fair that it be enabled by default, I don't see any web server going without it. Which is why I re-enabled it by default in libasync directly.

@soarqin

This comment has been minimized.

Show comment
Hide comment
@soarqin

soarqin Jan 21, 2016

Contributor

@etcimon

  1. port reuse is restricted to same user, setsockopt would fail if listen to the same port from another user
  2. if the socket listend earlier was not set SO_REUSEPORT, setsockopt would fail
    These 2 failures are not the same as that of listen errors without port reusing, although it is okay if we don't care about different types of socket errors
Contributor

soarqin commented Jan 21, 2016

@etcimon

  1. port reuse is restricted to same user, setsockopt would fail if listen to the same port from another user
  2. if the socket listend earlier was not set SO_REUSEPORT, setsockopt would fail
    These 2 failures are not the same as that of listen errors without port reusing, although it is okay if we don't care about different types of socket errors
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 21, 2016

Member

It's also important to keep in mind that not all servers are HTTP servers, and in practice there are a lot of HTTP servers, too, which get along without request distribution and which wouldn't work even correctly in that case. I'd instead prominently document this approach.

BTW, it probably makes sense to deprecate the (unsafe) TCPOptions.distribute and HTTPServerOptions.distribute options. runWorkerTask({ listenHTTP(...); }) together with SO_REUSEPORT would then be the direct replacement.

Member

s-ludwig commented Jan 21, 2016

It's also important to keep in mind that not all servers are HTTP servers, and in practice there are a lot of HTTP servers, too, which get along without request distribution and which wouldn't work even correctly in that case. I'd instead prominently document this approach.

BTW, it probably makes sense to deprecate the (unsafe) TCPOptions.distribute and HTTPServerOptions.distribute options. runWorkerTask({ listenHTTP(...); }) together with SO_REUSEPORT would then be the direct replacement.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jan 21, 2016

Contributor

if the socket listend earlier was not set SO_REUSEPORT, setsockopt would fail

I'm not sure I understand correctly. Which setsockopt command would fail?

in practice there are a lot of HTTP servers, too, which get along without request distribution and which wouldn't work even correctly in that case. I'd instead prominently document this approach.

Agree, it will have to be documented a bit more. Is there a downside to having it on by default though? The only difference is that the application won't crash at start if another one was already listening

Contributor

etcimon commented Jan 21, 2016

if the socket listend earlier was not set SO_REUSEPORT, setsockopt would fail

I'm not sure I understand correctly. Which setsockopt command would fail?

in practice there are a lot of HTTP servers, too, which get along without request distribution and which wouldn't work even correctly in that case. I'd instead prominently document this approach.

Agree, it will have to be documented a bit more. Is there a downside to having it on by default though? The only difference is that the application won't crash at start if another one was already listening

@soarqin

This comment has been minimized.

Show comment
Hide comment
@soarqin

soarqin Jan 21, 2016

Contributor

@etcimon If first socket on the port was not set SO_REUSEPORT, try to set it on the later ones would fail, which just the same semantic as SO_REUSEADDR

Contributor

soarqin commented Jan 21, 2016

@etcimon If first socket on the port was not set SO_REUSEPORT, try to set it on the later ones would fail, which just the same semantic as SO_REUSEADDR

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 22, 2016

Member

It can lead to unexpected and hard to detect behavior if a previous program instance keeps running in the background and may lead to data corruption if the application isn't ready for concurrency. For example it may lose data when doing a read-modify-write cycle for on-disk data without locking. I'd just want to keep enabling non-obvious behavior explicit. If you need to scale a service, you have to put work into that and adding an options |= HTTPServerOption.reusePort; isn't really worth mentioning.

Member

s-ludwig commented Jan 22, 2016

It can lead to unexpected and hard to detect behavior if a previous program instance keeps running in the background and may lead to data corruption if the application isn't ready for concurrency. For example it may lose data when doing a read-modify-write cycle for on-disk data without locking. I'd just want to keep enabling non-obvious behavior explicit. If you need to scale a service, you have to put work into that and adding an options |= HTTPServerOption.reusePort; isn't really worth mentioning.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jan 22, 2016

Contributor

True, I'll disable the default for the next version =)

Contributor

etcimon commented Jan 22, 2016

True, I'll disable the default for the next version =)

@soarqin

This comment has been minimized.

Show comment
Hide comment
@soarqin

soarqin Jan 25, 2016

Contributor

I removed reusePort from default options, wait for new release of libasync then.

Contributor

soarqin commented Jan 25, 2016

I removed reusePort from default options, wait for new release of libasync then.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 29, 2016

Member

Okay, thanks! I'll merge now (the libasync driver can be adjusted later).

Member

s-ludwig commented Jan 29, 2016

Okay, thanks! I'll merge now (the libasync driver can be adjusted later).

s-ludwig added a commit that referenced this pull request Jan 29, 2016

Merge pull request #1379 from soarqin/reuse_port
add kernel-level distribution support(SO_REUSEPORT) for linux

@s-ludwig s-ludwig merged commit d51662e into vibe-d:master Jan 29, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment