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

Better type safety with Target.SendMessageToTarget #10

Closed
matthewmueller opened this issue Jul 3, 2017 · 7 comments
Closed

Better type safety with Target.SendMessageToTarget #10

matthewmueller opened this issue Jul 3, 2017 · 7 comments

Comments

@matthewmueller
Copy link

matthewmueller commented Jul 3, 2017

Right now Target.SendMessageToTarget accepts Message as a string, which is what the protocol expects, but that string is actually a subcommand, like this:

bctx, err := c.Target.CreateBrowserContext(ctx)
if err != nil {
	panic(err)
}

width := 940
height := 500
tar, err := c.Target.CreateTarget(ctx, &cdpcmd.TargetCreateTargetArgs{
	BrowserContextID: &bctx.BrowserContextID,
	URL:              "about:blank",
	Width:            &width,
	Height:           &height,
})
if err != nil {
	panic(err)
}

attached, err := c.Target.AttachToTarget(ctx, &cdpcmd.TargetAttachToTargetArgs{
	TargetID: tar.TargetID,
})
if err != nil {
	panic(err)
} else if !attached.Success {
	panic("could not attach")
}

err = c.Target.SendMessageToTarget(ctx, &cdpcmd.TargetSendMessageToTargetArgs{
	TargetID: tar.TargetID,
	Message:  `{"id":0,"method":"Page.enable","params":{}}`,
})
if err != nil {
	panic(err)
}

I'm not exactly sure how this will work in Goland, but it'd be nice to somehow treat Message as a recursive command that runs json.Marshal on the command before sending it off.

@mafredri
Copy link
Owner

mafredri commented Jul 3, 2017

Oh, that's interesting. Thanks for bringing this to my attention. I've never tried to use the SendMessageToTarget API before.

I think we definitely can do better here, and I have a few ideas but they will need some more thought.

Some ideas:

  • Change SendMessageToTarget return signature from error to (*cdp.Client, error).
    • Usage: msgClient.Page.Enable(ctx)
  • Change SendMessageToTarget return signature from error to (context.Context, error).
    • Usage: c.Page.Enable(messageCtx)
  • Add a execution method to *Args
    • Usage: c.Target.SendMessageToTarget(ctx, &cdpcmd.TargetSendMessageToTargetArgs{TargetID: tar.TargetID, Message: &cdpcmd.PageEnableArgs{}) (messages is of type Doer interface{ Do(...) })
    • Con: Requires creating args for methods that take no args, e.g. currently there is no PageEnableArgs.
    • Con: Exposes multiple ways of doing things, e.g. args.Do(ctx, conn) or c.Network.Enable(ctx, args)
  • Leave c.Target.SendMessageToTarget as-is (low-level) and create a new helper function/API to deal with the communication

By the way, doesn't c.Target.AttachToTarget() switch the target for the current connection? So calling c.Page.Enable() would actually enable the Page domain on the target we just attached to, or do I misremember?

@matthewmueller
Copy link
Author

matthewmueller commented Jul 3, 2017

By the way, doesn't c.Target.AttachToTarget() switch the target for the current connection? So calling c.Page.Enable() would actually enable the Page domain on the target we just attached to, or do I misremember?

Wow thank you! This probably explains why my sessions have been a bit strange.

My goal was to reuse a single websocket connection concurrently, but I'm not seeing how it's possible to get Target.receivedMessageFromTarget events without attaching. So I think the workaround there is to connect to each target separately.

Which in that case I'd vote for:

Leave c.Target.SendMessageToTarget as-is (low-level) and create a new helper function/API to deal with the communication

Since it provides the best parity between the protocol and this library.

@mafredri
Copy link
Owner

mafredri commented Jul 3, 2017

My goal was to reuse a single websocket connection concurrently, but I'm not seeing how it's possible to get Target.receivedMessageFromTarget events without attaching. So I think the workaround there is to connect to each target separately.

It seems you can communicate with any target as long as you have attached to it once before, so it should be fully possible to reuse one connection for all targets, it just means that communication must be synchronized between SendMessageToTarget and ReceivedMessageFromTarget and keeping track of which target the connection is attached to when using the *cdp.Client.

Here's what I used, slightly modified from your original code sample:

Multi-target communication
bctx, err := c.Target.CreateBrowserContext(ctx)
if err != nil {
	return err
}

width := 940
height := 500
tar1, err := c.Target.CreateTarget(ctx, &cdpcmd.TargetCreateTargetArgs{
	BrowserContextID: &bctx.BrowserContextID,
	URL:              "about:blank",
	Width:            &width,
	Height:           &height,
})
if err != nil {
	return err
}
tar2, err := c.Target.CreateTarget(ctx, &cdpcmd.TargetCreateTargetArgs{
	BrowserContextID: &bctx.BrowserContextID,
	URL:              "about:blank",
	Width:            &width,
	Height:           &height,
})
if err != nil {
	return err
}

recvMessage, err := c.Target.ReceivedMessageFromTarget(ctx)
if err != nil {
	return err
}

go func() {
	defer recvMessage.Close()

	for {
		ev, err := recvMessage.Recv()
		if err != nil {
			log.Println(err)
			return
		}
		log.Printf("%#v", ev)
	}
}()

attached, err := c.Target.AttachToTarget(ctx, &cdpcmd.TargetAttachToTargetArgs{
	TargetID: tar1.TargetID,
})
if err != nil {
	return err
} else if !attached.Success {
	return errors.New("could not attach")
}

attached, err = c.Target.AttachToTarget(ctx, &cdpcmd.TargetAttachToTargetArgs{
	TargetID: tar2.TargetID,
})
if err != nil {
	return err
} else if !attached.Success {
	return errors.New("could not attach")
}

err = c.Target.SendMessageToTarget(ctx, &cdpcmd.TargetSendMessageToTargetArgs{
	TargetID: tar1.TargetID,
	Message:  `{"id":0,"method":"Page.enable","params":{}}`,
})
if err != nil {
	return err
}

err = c.Target.SendMessageToTarget(ctx, &cdpcmd.TargetSendMessageToTargetArgs{
	TargetID: tar2.TargetID,
	Message:  `{"id":0,"method":"Page.enable","params":{}}`,
})
if err != nil {
	return err
}
Log output
rpcc 2017/07/03 17:08:07 => rpc_id=1 rpc_method=Target.createBrowserContext data={"id":1,"method":"Target.createBrowserContext"}

rpcc 2017/07/03 17:08:07 <= rpc_id=1 rpc_event=false data={"id":1,"result":{"browserContextId":"1587e259-a5e0-43ed-8144-a5f136e2ad73"}}
rpcc 2017/07/03 17:08:07 => rpc_id=2 rpc_method=Target.createTarget data={"id":2,"method":"Target.createTarget","params":{"url":"about:blank","width":940,"height":500,"browserContextId":"1587e259-a5e0-43ed-8144-a5f136e2ad73"}}

rpcc 2017/07/03 17:08:07 <= rpc_id=2 rpc_event=false data={"id":2,"result":{"targetId":"e5d17a02-4fe1-4f75-a628-d2254c1dbed6"}}
rpcc 2017/07/03 17:08:07 => rpc_id=3 rpc_method=Target.createTarget data={"id":3,"method":"Target.createTarget","params":{"url":"about:blank","width":940,"height":500,"browserContextId":"1587e259-a5e0-43ed-8144-a5f136e2ad73"}}

rpcc 2017/07/03 17:08:07 <= rpc_id=3 rpc_event=false data={"id":3,"result":{"targetId":"b5bdcd48-6f58-47da-a73f-d654eb901c86"}}
rpcc 2017/07/03 17:08:07 => rpc_id=4 rpc_method=Target.attachToTarget data={"id":4,"method":"Target.attachToTarget","params":{"targetId":"e5d17a02-4fe1-4f75-a628-d2254c1dbed6"}}

rpcc 2017/07/03 17:08:07 <= rpc_id=0 rpc_event=true data={"method":"Target.attachedToTarget","params":{"targetInfo":{"targetId":"e5d17a02-4fe1-4f75-a628-d2254c1dbed6","type":"page","title":"","url":"about:blank"},"waitingForDebugger":false}}
rpcc 2017/07/03 17:08:07 <= rpc_id=4 rpc_event=false data={"id":4,"result":{"success":true}}
rpcc 2017/07/03 17:08:07 => rpc_id=5 rpc_method=Target.attachToTarget data={"id":5,"method":"Target.attachToTarget","params":{"targetId":"b5bdcd48-6f58-47da-a73f-d654eb901c86"}}

rpcc 2017/07/03 17:08:07 <= rpc_id=0 rpc_event=true data={"method":"Target.attachedToTarget","params":{"targetInfo":{"targetId":"b5bdcd48-6f58-47da-a73f-d654eb901c86","type":"page","title":"","url":"about:blank"},"waitingForDebugger":false}}
rpcc 2017/07/03 17:08:07 <= rpc_id=5 rpc_event=false data={"id":5,"result":{"success":true}}
rpcc 2017/07/03 17:08:07 => rpc_id=6 rpc_method=Target.sendMessageToTarget data={"id":6,"method":"Target.sendMessageToTarget","params":{"targetId":"e5d17a02-4fe1-4f75-a628-d2254c1dbed6","message":"{\"id\":0,\"method\":\"Page.enable\",\"params\":{}}"}}

rpcc 2017/07/03 17:08:07 <= rpc_id=6 rpc_event=false data={"id":6,"result":{}}
rpcc 2017/07/03 17:08:07 => rpc_id=7 rpc_method=Target.sendMessageToTarget data={"id":7,"method":"Target.sendMessageToTarget","params":{"targetId":"b5bdcd48-6f58-47da-a73f-d654eb901c86","message":"{\"id\":0,\"method\":\"Page.enable\",\"params\":{}}"}}

rpcc 2017/07/03 17:08:07 <= rpc_id=7 rpc_event=false data={"id":7,"result":{}}
rpcc 2017/07/03 17:08:07 <= rpc_id=0 rpc_event=true data={"method":"Target.receivedMessageFromTarget","params":{"targetId":"e5d17a02-4fe1-4f75-a628-d2254c1dbed6","message":"{\"id\":0,\"result\":{}}"}}
2017/07/03 17:08:07 &cdpevent.TargetReceivedMessageFromTargetReply{TargetID:"e5d17a02-4fe1-4f75-a628-d2254c1dbed6", Message:"{\"id\":0,\"result\":{}}"}
rpcc 2017/07/03 17:08:07 <= rpc_id=0 rpc_event=true data={"method":"Target.receivedMessageFromTarget","params":{"targetId":"b5bdcd48-6f58-47da-a73f-d654eb901c86","message":"{\"id\":0,\"result\":{}}"}}
2017/07/03 17:08:07 &cdpevent.TargetReceivedMessageFromTargetReply{TargetID:"b5bdcd48-6f58-47da-a73f-d654eb901c86", Message:"{\"id\":0,\"result\":{}}"}

@mafredri
Copy link
Owner

mafredri commented Jul 3, 2017

Actually, it seems my speculation about c.Target.AttachToTarget was incorrect. The commands are sent to the original page.WebSocketDebuggerURL that *rpcc.Conn is connected to, even after creating and attaching to new targets. Sorry about that.

With the code I posted above, I can do the following without problems:

c.Page.Enable(ctx)
c.Page.Navigate(ctx, cdpcmd.NewPageNavigateArgs("https://github.com"))

err = c.Target.SendMessageToTarget(ctx, &cdpcmd.TargetSendMessageToTargetArgs{
	TargetID: tar1.TargetID,
	Message:  `{"id":1,"method":"Page.navigate","params":{"url":"https://www.google.com"}}`,
})
if err != nil {
	return err
}

err = c.Target.SendMessageToTarget(ctx, &cdpcmd.TargetSendMessageToTargetArgs{
	TargetID: tar2.TargetID,
	Message:  `{"id":1,"method":"Page.navigate","params":{"url":"https://www.duckduckgo.com"}}`,
})
if err != nil {
	return err
}

@mafredri
Copy link
Owner

mafredri commented Jul 4, 2017

I got inspired and hacked together something that works with the current implementation. A bunch of code has been omitted for brevity, but it's based off earlier code samples.

This logic could easily be encapsulated by the lib and exposed as a helper. Think tc := cdp.ClientForTarget(target.TargetID) or similar, I'll have to think about what it would look like. Benefit here being that we do not need to establish multiple websocket connections and we can re-use all the logic in rpcc for managing events.

Considering this, I think it would be beneficial to change cdpcmd.TargetSendMessageToTargetArgs.Message from string to []byte and similarly cdpevent.TargetReceivedMessageFromTargetReply.Message from string to json.RawMessage.

Thoughts?

Please note that this is prototype code, and e.g. make(chan []byte, 1) is hardly optimal 😅.

// targetCodec sends messages to a target via the provided client.
type targetCodec struct {
	ctx      context.Context
	client   *cdp.Client
	r        chan []byte
	targetID cdptype.TargetID
}

func (c *targetCodec) WriteRequest(r *rpcc.Request) error {
	b, err := json.Marshal(r)
	if err != nil {
		return err
	}
	args := &cdpcmd.TargetSendMessageToTargetArgs{
		TargetID: c.targetID,
		Message:  string(b),
	}
	return c.client.Target.SendMessageToTarget(c.ctx, args)
}

func (c *targetCodec) ReadResponse(r *rpcc.Response) error {
	b := <-c.r
	return json.Unmarshal(b, r)
}

func run() error {
	// Setup conn, client...

	// Start listening to target messages.
	recvMessage, err := c.Target.ReceivedMessageFromTarget(ctx)
	if err != nil {
		return err
	}

	// Handle incoming messages from all targets.
	tar1Ch := make(chan []byte, 1)
	tar2Ch := make(chan []byte, 1)
	go func() {
		defer recvMessage.Close()

		for {
			ev, err := recvMessage.Recv()
			if err != nil {
				log.Println(err)
				return
			}
			if ev.TargetID == tar1.TargetID {
				tar1Ch <- []byte(ev.Message)
			}
			if ev.TargetID == tar2.TargetID {
				tar2Ch <- []byte(ev.Message)
			}
		}
	}()

	// Attach to targets...

	// Setup a new fake connection to target.
	tar1Conn, err := rpcc.Dial(
		"", // Ignore URL, we're not connecting anywhere.
		rpcc.WithDialer(func(_ context.Context, _ string) (net.Conn, error) { return nil, nil }),
		rpcc.WithCodec(func(conn io.ReadWriter) rpcc.Codec {
			return &targetCodec{
				ctx:      ctx,
				r:        tar1Ch,
				targetID: tar1.TargetID,
				client:   c,
			}
		}),
	)
	if err != nil {
		return err
	}
	defer tar1Conn.Close()

	// Create cdp client.
	tar1Client := cdp.NewClient(tar1Conn)

	// Use as any client.
	loadEventFired, err := tar1Client.Page.LoadEventFired(ctx)
	if err != nil {
		return err
	}
	tar1Client.Page.Enable(ctx)
	tar1Client.Page.Navigate(ctx, cdpcmd.NewPageNavigateArgs("https://www.google.com"))
	loadEv, err := loadEventFired.Recv()
	if err != nil {
		return err
	}
	log.Printf("Load event at %v", loadEv.Timestamp.Time())

	// ...
	
	return nil
}

@matthewmueller
Copy link
Author

Thanks for the implementation! I'll be going through this more in detail today. One thing I ran into with the single connection was that it was working pretty well on OSX, but on linux it failed immediately. I haven't traced the root cause, but it seems related to this attachTarget stuff. More details here: https://groups.google.com/a/chromium.org/d/msg/headless-dev/qqbZVZ2IwEw/P-yKHQ-JAwAJ

I also realized that while multiple connections is less than ideal in a real-world setting, the number of targets will be quite limited (potentially 10s, but not 100s) on a single chrome process.

@mafredri
Copy link
Owner

@matthewmueller type safety is landing in #41! Check it out if you're interested 😄!

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

No branches or pull requests

2 participants