Allow to set custom WS URL waiting time #1036

karelbilek · 2022-03-15T03:47:41Z

With a heavy CPU utilization, Chrome sometimes does not start in 20 seconds.

This adds a custom option to set the WS URL waiting time.

ZekeLu · 2022-03-15T10:51:39Z

Suggested changes:

Add a commit to change the arrangement of the ExecAllocator struct. wg works like a mutex hat here.

diff --git a/allocate.go b/allocate.go
index f639ea2..3de99b1 100644
--- a/allocate.go
+++ b/allocate.go
@@ -105,11 +105,11 @@ type ExecAllocator struct {
    initFlags map[string]interface{}
    initEnv   []string
 
+   modifyCmdFunc func(cmd *exec.Cmd)
+
    wg sync.WaitGroup
 
    combinedOutputWriter io.Writer
-
-   modifyCmdFunc func(cmd *exec.Cmd)
 }

Add another commit to implement WSURLReadTimeout:

diff --git a/allocate.go b/allocate.go
index 3de99b1..1958308 100644
--- a/allocate.go
+++ b/allocate.go
@@ -37,7 +37,8 @@ type Allocator interface {
 // to create the allocator without the unnecessary context layer.
 func setupExecAllocator(opts ...ExecAllocatorOption) *ExecAllocator {
    ep := &ExecAllocator{
-       initFlags: make(map[string]interface{}),
+       initFlags:        make(map[string]interface{}),
+       wsURLReadTimeout: 20 * time.Second,
    }
    for _, o := range opts {
        o(ep)
@@ -107,6 +108,11 @@ type ExecAllocator struct {
 
    modifyCmdFunc func(cmd *exec.Cmd)
 
+   // Chrome will sometimes fail to print the websocket, or run for a long
+   // time, without properly exiting. To avoid blocking forever in those
+   // cases, give up after a specified timeout.
+   wsURLReadTimeout time.Duration
+
    wg sync.WaitGroup
 
    combinedOutputWriter io.Writer
@@ -224,11 +230,6 @@ func (a *ExecAllocator) Allocate(ctx context.Context, opts ...BrowserOption) (*B
        close(c.allocated)
    }()
 
-   // Chrome will sometimes fail to print the websocket, or run for a long
-   // time, without properly exiting. To avoid blocking forever in those
-   // cases, give up after twenty seconds.
-   const wsURLReadTimeout = 20 * time.Second
-
    var wsURL string
    wsURLChan := make(chan struct{}, 1)
    go func() {
@@ -237,7 +238,7 @@ func (a *ExecAllocator) Allocate(ctx context.Context, opts ...BrowserOption) (*B
    }()
    select {
    case <-wsURLChan:
-   case <-time.After(wsURLReadTimeout):
+   case <-time.After(a.wsURLReadTimeout):
        err = errors.New("websocket url timeout reached")
    }
    if err != nil {
@@ -494,6 +495,14 @@ func CombinedOutput(w io.Writer) ExecAllocatorOption {
    }
 }
 
+// WSURLReadTimeout sets the waiting time for reading the WebSocket URL.
+// The default value is 20 seconds.
+func WSURLReadTimeout(t time.Duration) ExecAllocatorOption {
+   return func(a *ExecAllocator) {
+       a.wsURLReadTimeout = t
+   }
+}
+
 // NewRemoteAllocator creates a new context set up with a RemoteAllocator,
 // suitable for use with NewContext. The url should point to the browser's
 // websocket address, such as "ws://127.0.0.1:$PORT/devtools/browser/...".

karelbilek · 2022-03-15T11:40:45Z

Will do

With a heavy CPU utilization, Chrome sometimes does not start in 20 seconds. This adds a custom option to set the WS URL waiting time.

karelbilek · 2022-03-16T12:59:04Z

Commit updated..

ZekeLu · 2022-03-16T15:15:11Z

allocate.go

@@ -105,11 +106,15 @@ type ExecAllocator struct {
 	initFlags map[string]interface{}
 	initEnv   []string

-	wg sync.WaitGroup
+	modifyCmdFunc func(cmd *exec.Cmd)


Please make this change into its own commit. And explain why the change.

See this comment: #1036 (comment)

ZekeLu · 2022-03-16T15:16:35Z

allocate.go


-	modifyCmdFunc func(cmd *exec.Cmd)
+	wg                   sync.WaitGroup


Do not remove the blank line after wg sync.WaitGroup because wg is not totally for combinedOutputWriter.

reference:

chromedp/allocate.go

Lines 206 to 209 in 621263b

a.wg.Add(1) // for the entire allocator

if a.combinedOutputWriter != nil {

a.wg.Add(1) // for the io.Copy in a separate goroutine

}

II guess I just do not understand your request 1. sorry :(

You said wg works like a "mutex hat". Reading about mutex hats, from your link - "It sits, like a hat, on top of the variables that it protects."

What does it protect here?

I just don't understand what is the purpose of moving modifyCmdFunc func(cmd *exec.Cmd) up. And if it should be a separate commit, what should be reason for that in that commit.

Sorry, it's my fault that I haven't made it clear. First of all, this is a personal taste. So it's a little hard to understand. I will try to explain it.

First, see the source code below:

chromedp/allocate.go

Lines 206 to 225 in 621263b

a.wg.Add(1) // for the entire allocator

if a.combinedOutputWriter != nil {

a.wg.Add(1) // for the io.Copy in a separate goroutine

}

go func() {

// First wait for the process to be finished.

// TODO: do we care about this error in any scenario? if the

// user cancelled the context and killed chrome, this will most

// likely just be "signal: killed", which isn't interesting.

cmd.Wait()

// Then delete the temporary user data directory, if needed.

if removeDir {

if err := os.RemoveAll(dataDir); c.cancelErr == nil {

c.cancelErr = err

}

}

a.wg.Done()

close(c.allocated)

}()

chromedp/allocate.go

Lines 234 to 237 in 621263b

go func() {

wsURL, err = readOutput(stdout, a.combinedOutputWriter, a.wg.Done)

wsURLChan <- struct{}{}

}()

chromedp/allocate.go

Lines 244 to 248 in 621263b

if a.combinedOutputWriter != nil {

// There's no io.Copy goroutine to call the done func.

// TODO: a cleaner way to deal with this edge case?

a.wg.Done()

}

As the comments in line 206 and 208 said, wg is for both the entire allocator and the io.Copy in a separate goroutine. That's why I said wg works like a mutex hat. Strictly speaking, It does not protect combinedOutputWriter, but it has something to do with combinedOutputWriter. So I think they should be arranged together in the struct.

On the other hand, wg is not totally for combinedOutputWriter (it's for the entire allocator too). If we remove the blank line between them, it will make it looks like that wg is totally for combinedOutputWriter.

So I think the best choice is to put wg and combinedOutputWriter into the last part of the struct and leave a blank line between them. This is what it's arranged before modifyCmdFunc func(cmd *exec.Cmd) is added in #674.

This suggested change has nothing to do with wsURLReadTimeout, and it's hard to explain. That's why it deserves a standalone commit.

@ZekeLu If this change has nothing to do with wsURLReadTimeout, then why it is even discussed in this PR?
upd. Sorry, I've meant modifyCmdFunc

@ZekeLu If this change has nothing to do with wsURLReadTimeout, then why it is even discussed in this PR?

I have submitted 1c2b99f so we can ignore this part now.

karelbilek · 2022-03-16T15:20:39Z

ah sorry, then I guess I misunderstood

…

On Wed, 16 Mar 2022 at 22:19 Zeke Lu ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In allocate.go <#1036 (comment)>: > @@ -105,11 +106,15 @@ type ExecAllocator struct { initFlags map[string]interface{} initEnv []string - wg sync.WaitGroup + modifyCmdFunc func(cmd *exec.Cmd) Please make this change into its own commit. And explain why the change. ------------------------------ In allocate.go <#1036 (comment)>: > - modifyCmdFunc func(cmd *exec.Cmd) + wg sync.WaitGroup Do not remove the blank line after wg sync.WaitGroup because wg is not totally for combinedOutputWriter. — Reply to this email directly, view it on GitHub <#1036 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZT4JS5LXCNX4AWWCCZCLVAH3XDANCNFSM5QXNWKJA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

waybackarchiver · 2022-03-20T04:06:22Z

Any news?

karelbilek · 2022-03-24T08:05:08Z

@waybackarchiver hey. I realized this is a bit nonsensical; when the CPU usage is high, and chrome does not start in 20 seconds, in my experience, it still doesn't start in 1 minute or 2 minutes or ever; so this PR is a bit useless.

I still think it should be user-settable though, as the 20 seconds timeout is a bit random and should not be hardcoded; but I don't have it as priority 1 to merge this :)

8tomat8 · 2022-08-10T12:50:10Z

@karelbilek are you planning to finish this PR or should we take it over?

ubombi · 2022-08-10T14:31:32Z

@karelbilek

hey. I realized this is a bit nonsensical; when the CPU usage is high, and chrome does not start in 20 seconds, in my experience, it still doesn't start in 1 minute or 2 minutes or ever; so this PR is a bit useless.

In AWS lambda, during cold start is occasionally takes like ~30 sec or more. And start each time, having this hard-coded artificial timeout removed from the source code.

ZekeLu · 2022-08-11T02:59:34Z

I'm going to cherry-pick the changes.

ZekeLu · 2022-08-11T03:00:41Z

done.

8tomat8 · 2022-08-11T12:10:06Z

@ZekeLu When is the next release planned? I want to plan switch on this repo from our fork

ZekeLu · 2022-08-14T05:57:22Z

@8tomat8 v0.8.4 has just been released.

Allow to set custom WS URL waiting time

5fdaa4e

With a heavy CPU utilization, Chrome sometimes does not start in 20 seconds. This adds a custom option to set the WS URL waiting time.

karelbilek force-pushed the wsurlread branch from 5447412 to 5fdaa4e Compare March 16, 2022 12:57

ZekeLu requested changes Mar 16, 2022

View reviewed changes

ZekeLu mentioned this pull request Aug 10, 2022

Make chrome startup timeout configurable #1134

Closed

ZekeLu closed this Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to set custom WS URL waiting time #1036

Allow to set custom WS URL waiting time #1036

karelbilek commented Mar 15, 2022

ZekeLu commented Mar 15, 2022 •

edited

karelbilek commented Mar 15, 2022

karelbilek commented Mar 16, 2022

ZekeLu Mar 16, 2022

ZekeLu Mar 16, 2022

ZekeLu Mar 16, 2022

ZekeLu Mar 16, 2022

karelbilek Mar 24, 2022

ZekeLu Mar 24, 2022

8tomat8 Aug 10, 2022 •

edited

ZekeLu Aug 11, 2022

karelbilek commented Mar 16, 2022 via email

waybackarchiver commented Mar 20, 2022

karelbilek commented Mar 24, 2022

8tomat8 commented Aug 10, 2022

ubombi commented Aug 10, 2022 •

edited

ZekeLu commented Aug 11, 2022

ZekeLu commented Aug 11, 2022

8tomat8 commented Aug 11, 2022

ZekeLu commented Aug 14, 2022

	a.wg.Add(1) // for the entire allocator
	if a.combinedOutputWriter != nil {
	a.wg.Add(1) // for the io.Copy in a separate goroutine
	}

	go func() {
	wsURL, err = readOutput(stdout, a.combinedOutputWriter, a.wg.Done)
	wsURLChan <- struct{}{}
	}()

	if a.combinedOutputWriter != nil {
	// There's no io.Copy goroutine to call the done func.
	// TODO: a cleaner way to deal with this edge case?
	a.wg.Done()
	}

Allow to set custom WS URL waiting time #1036

Allow to set custom WS URL waiting time #1036

Conversation

karelbilek commented Mar 15, 2022

ZekeLu commented Mar 15, 2022 • edited

karelbilek commented Mar 15, 2022

karelbilek commented Mar 16, 2022

ZekeLu Mar 16, 2022

Choose a reason for hiding this comment

ZekeLu Mar 16, 2022

Choose a reason for hiding this comment

ZekeLu Mar 16, 2022

Choose a reason for hiding this comment

ZekeLu Mar 16, 2022

Choose a reason for hiding this comment

karelbilek Mar 24, 2022

Choose a reason for hiding this comment

ZekeLu Mar 24, 2022

Choose a reason for hiding this comment

8tomat8 Aug 10, 2022 • edited

Choose a reason for hiding this comment

ZekeLu Aug 11, 2022

Choose a reason for hiding this comment

karelbilek commented Mar 16, 2022 via email

waybackarchiver commented Mar 20, 2022

karelbilek commented Mar 24, 2022

8tomat8 commented Aug 10, 2022

ubombi commented Aug 10, 2022 • edited

ZekeLu commented Aug 11, 2022

ZekeLu commented Aug 11, 2022

8tomat8 commented Aug 11, 2022

ZekeLu commented Aug 14, 2022

ZekeLu commented Mar 15, 2022 •

edited

8tomat8 Aug 10, 2022 •

edited

ubombi commented Aug 10, 2022 •

edited