Skip to content

Conversation

@patterniha
Copy link
Collaborator

@patterniha patterniha commented Sep 2, 2025

  1. because we don't have UplinkOnly/DownlinkOnly, after outbound-connection is closed(response returned), request function(and as a result inbound-connection) remain open for a long time(especially when the network is TCP)

///

  1. we have a weird code in:

    return len(b), nil

    it should be: return 0, err(dns-query is not sent but we are waiting for a response!)

@patterniha patterniha force-pushed the fix-proxy-dns branch 2 times, most recently from 1ad1d72 to 6a77ced Compare September 2, 2025 17:50
@RPRX
Copy link
Member

RPRX commented Sep 3, 2025

b, err := reader.ReadMessage() 没有传入 ctx,不还是会在这一行卡着吗?

@patterniha
Copy link
Collaborator Author

b, err := reader.ReadMessage()If ctx is not passed in, won't it still be stuck on this line?

after connIdle-timeout exceed -> ctx is canceled ->task.run ends ->Process ends ->

  1. if link.reader is pipe, we have interrupt(reader) after Process in proxyman/outbound/handler.go
  2. else if it is inbound-reader(dispatchLink) -> dispatchLink ends -> inbound-Process ends -> inbound-conn closed -> reader closed

so in both case reader is closed and there is no problem.

@patterniha
Copy link
Collaborator Author

ready.

@patterniha
Copy link
Collaborator Author

patterniha commented Sep 3, 2025

also, there is no need for go h.handleIPQuery(id, qType, domain, writer) or go h.rejectNonIPQuery(id, qType, domain, writer), there is no need to run them in new goroutine, we can simply do h.handleIPQuery(id, qType, domain, writer) or h.rejectNonIPQuery(id, qType, domain, writer), i will fix change it .

@Fangliding
Copy link
Member

好好的异步查询响应本来就可以乱序回又突发奇想了是吧。。。 你改点什么就是fix 然后说我fix了好几十个bug

@patterniha
Copy link
Collaborator Author

patterniha commented Sep 3, 2025

i said there is no need to run in new goroutine, write instantly returned.
so it is better not to make a new goroutine, this is not a bug, just optimization, i didn't say this a bug.

it is better to call it "change" instead of "fix"

@patterniha
Copy link
Collaborator Author

also, there is no need for go h.handleIPQuery(id, qType, domain, writer) or go h.rejectNonIPQuery(id, qType, domain, writer), there is no need to run them in new goroutine, we can simply do h.handleIPQuery(id, qType, domain, writer) or h.rejectNonIPQuery(id, qType, domain, writer), i will fix change it .

done.

@Fangliding
Copy link
Member

Fangliding commented Sep 3, 2025

i said there is no need to run in new goroutine, write instantly returned. so it is better not to make a new goroutine, this is not a bug, just optimization, i didn't say this a bug.

it is better to call it "change" instead of "fix"

你optimize了个啥 一个goroutine而已 dns本来就可以乱序处理是对的 就是些无意义改动

@patterniha
Copy link
Collaborator Author

patterniha commented Sep 3, 2025

the main bug in this PR was return len(b), nil, and after outbound-connection is closed(or dial failed), inbound-connection remained open for 300s .

also, there is no need for new goroutines, we should simplify the code.

///
@Fangliding

also, outbound-dns has userLevel option:

UserLevel uint32 `json:"userLevel"`

but this is forgotten in CN-doc, please add it, thx.

@patterniha
Copy link
Collaborator Author

patterniha commented Sep 4, 2025

@RPRX

@Fangliding is right, because handleIPQuery use built-in-DNS and it may take a while to answer, it is better to run it in new goroutine.
but for rejectNonIPQuery there is no need for new goroutine.

so only handleIPQuery should run in goroutine, wait...

@patterniha
Copy link
Collaborator Author

so only handleIPQuery should run in goroutine, wait...

done and ready.

At first I didn't understand what @Fangliding meant

@RPRX RPRX changed the title DNS proxy: Fix some issues DNS outbound: Fix some issues Sep 4, 2025
@RPRX RPRX merged commit 53bd06a into XTLS:main Sep 4, 2025
39 checks passed
RPRX pushed a commit that referenced this pull request Sep 5, 2025
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.

4 participants