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

PortPool: add check port before alloc #72

Merged
merged 1 commit into from
Oct 9, 2016
Merged

Conversation

Akagi201
Copy link
Contributor

  • 申请端口前增加端口检查功能.
  • 每次申请端口从 v.ports 头部开始申请.
  • 释放端口放到 v.ports 头部, 这样充分复用端口, 让整个程序占用的端口范围更小.

@Akagi201
Copy link
Contributor Author

@winlinvip ping

(echo "build httplb" && cd httplb/ && go build . && echo "httlb ok") &&
(echo "build rtmplb" && cd rtmplb/ && go build . && echo "rtmplb ok") &&
(echo "build shell" && cd shell/ && go build . && echo "shell ok") &&
(echo "build apilb" && cd apilb/ && go build -ldflags "-s -w" . && echo "apilb ok") &&
Copy link
Member

Choose a reason for hiding this comment

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

这个PR是关于port监测的,这个改动和PR没有关系。应该撤销。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

host := ":" + strconv.Itoa(port)

// Try to create a server with the port
server, err := net.Listen("tcp", host)
Copy link
Member

Choose a reason for hiding this comment

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

server改成listener会更好

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

}
return v
}

func (v *PortPool) Alloc(nbPort int) (ports []int, err error) {
// Get alloc a port from the port pool
func (v *PortPool) Get() (int, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Get这个函数太宽泛了,改成非公开的 allocOnePort,会更好。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

for i, p := range v.ports {
if checkPort(p) {
// delete i-index element
v.ports = append(v.ports[:i], v.ports[i+1:]...)
Copy link
Member

Choose a reason for hiding this comment

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

这个地方可以改成:

  1. 取一个port出来,checkPort。从ports里面删除这个port。
  2. 如果port不可用,就添加到末尾。
  3. 如果可用,就返回。这样就不用used这个了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个 used 变量可以方便增加一个方法, 打印出我当前在使用哪些 port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我增加了一个方法 GetPortsInUse() 可以获取当前使用中的 port

Copy link
Member

Choose a reason for hiding this comment

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

为什么需要暴露这个函数呢:GetPortsInUse,这个是给谁用的?

@@ -105,21 +105,26 @@ func (v *PortPool) Free(port int) {
}
}

// GetPortsInUse get a list of port in use
func (v *PortPool) GetPortsInUse() []int {
Copy link
Member

Choose a reason for hiding this comment

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

为何要export这个函数?

Copy link
Member

Choose a reason for hiding this comment

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

另外,case 有failed:https://circleci.com/gh/ossrs/go-oryx/16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

一般的一个程序会占用一个端口, 启动时会配置进去, nc这个项目会占用多个端口, 而且是动态分配的, 需要提供个接口来查询哪些端口被占用了. 这样能了解当前系统占用了哪些端口.

@Akagi201
Copy link
Contributor Author

@winlinvip 已经都改好了, 可以看下.

@winlinvip
Copy link
Member

为什么需要暴露这个函数呢:GetPortsInUse,这个是给谁用的?

@Akagi201
Copy link
Contributor Author

@winlinvip 给运维看, 可以打印到日志里面, 或者提供接口查询. 从这个模块完整性上, 我觉得应该提供一个接口来查询当前分配了哪些端口.

@winlinvip
Copy link
Member

Great job~ 👍

@winlinvip winlinvip merged commit 7c0678b into ossrs:develop Oct 9, 2016
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.

None yet

2 participants