-
Notifications
You must be signed in to change notification settings - Fork 175
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
Added connection keeping feature. #291
base: main
Are you sure you want to change the base?
Added connection keeping feature. #291
Conversation
I'll take a look later today |
Hi, how is it going with the code, any revisions? |
Hi, I know it's quite some of code, and might cause potential bugs, but would be better if I can get somefeed back and do more test to or improve my code? It's been quite long. |
spec, err = StringToSpec(rt.JA3, rt.UserAgent, rt.forceHTTP1) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} |
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 should maybe raise an error if the JA3 isn't passed in or default it, otherwise it doesn't make a lot of sense to have the if len(rt.JA3) > 0 {
check
if p.clientHelloSpec != nil { | ||
p.establishWithClientHelloSpec() | ||
} else { | ||
p.establishWithClientHelloId() | ||
} | ||
|
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.
sequencing looks good
browser := &cycletls.Browser{ | ||
JA3: "", | ||
UserAgent: UserAgent, | ||
InsecureSkipVerify: true, | ||
} | ||
|
||
builder := cycletls.HttpClientBuilder{ | ||
Browser: browser, | ||
ClientHelloId: &utls.HelloRandomized, | ||
// ProxyUrl: "http://localhost:8888", // fiddler, watch if max tunnels reused | ||
MaxConnectionPerHost: 4, | ||
MaxIdleConnections: 10, | ||
Timeout: time.Second * 5, | ||
} | ||
|
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.
I don't mind adding these utilities to the golang side so things are more configurable. The question/issue is the implementation for this on the javascript side. Keep alive works this way on the golang side but for js the response won't be streamed, feel free to add these functions to the client file as added utilities. As long as this doesn't break any existing functionality I'll merge this in.
Just a quick summary, but move the utility functions from client_2.go into client.go and validate no functionality is broken. I just ran tests on your branch and they should rerun whenever you push updates, as long as they all pass this should be good to merge in. |
Thanks for the feed back, got a bunch of workload recenlty, will solve em when I'm free :) |
@neverusedname hope you are doing well |
Hi, I've almost done with implementing connection keeping feature.
I've exposed an HttpClientBuilder structure to help to build an http.Client which could keep the connection alive.
I am not sure if my code quality is ok so I put my code in a client2.go file, if you think the code ok, I may move my code from client2.go to client.go.
One of the possible bugs is that if the connection is closed by the remote server, the http.Client would not be able to notice the closed conn, and raise an unexpected EOF error, I actaully have no idea how to fix it.
I've also modified fhttp to support the connection keeping feature. I've