devtools: wrap TcpStream to synchronize network operations by brentschroeter · Pull Request #43472 · servo/servo

brentschroeter · 2026-03-20T07:44:17Z

Introduces a new type DevtoolsConnection, which implements the JsonPacketStream trait with proper coordination of I/O across threads. This replaces the implementation of JsonPacketStream for raw TcpStreams, which was susceptible to interleaving writes when cloned across threads.

DevtoolsConnection also defensively synchronizes read operations. These are less likely to cause issues: in practice actors should never independently pull incoming messages without centralized coordination.

Testing: No new tests, as interleaving writes are difficult to evoke deterministically. Removing the JsonPacketStream implementation for TcpStream should discourage regressions due to improper use of raw streams.

brentschroeter · 2026-03-20T07:46:50Z

Related to this discussion: #general > TcpStream::try_clone() thread safety in devtools. If it's helpful, I can copy the details into a formal bug report for posterity.

brentschroeter · 2026-03-20T07:52:56Z

A happy side effect is that this removes a big source of flakiness in the existing devtools tests. On my machine with the main branch, ./mach test-devtools fails at a rate of 5 times in 50, compared to 0 times in 50 with this branch. Potentially progress for #39273?

eerii

Awesome change, thank you so much! I like how it looks, I left a few nits / style comments but great code.

I am testing it locally and it seems to have also fixed the flakiness I had on my machine with some devtools tests c:

Signed-off-by: Brent Schroeter <contact@brentsch.com>

Co-authored-by: eri <eri@igalia.com> Signed-off-by: Brent Schroeter <contact@brentsch.com>

Signed-off-by: Brent Schroeter <contact@brentsch.com>

brentschroeter · 2026-03-25T23:30:21Z

@eerii thanks for the suggestions! Updated, and rebased on top of #43623. ~~Only comment I have not addressed in code is the one re: the DevtoolsConnection docstring—#43472 (comment).~~ (edit: now resolved)

Signed-off-by: Brent Schroeter <contact@brentsch.com>

eerii

Thank you so much, everything looks great! c:

Skgland · 2026-04-07T19:18:55Z

Testing: [..] Removing the JsonPacketStream implementation for TcpStream should discourage regressions due to improper use of raw streams.

This reads to me like impl JsonPacketStream for TcpStream should have been removed, but it wasn't?

servo/components/devtools/protocol.rs

Lines 39 to 84 in 66d232e

 impl JsonPacketStream for TcpStream { 

 fn write_json_packet<T: Serialize>(&mut self, message: &T) -> Result<(), ActorError> { 

 let s = serde_json::to_string(message).map_err(|_| ActorError::Internal)?; 

 debug!("<- {}", s); 

 write!(self, "{}:{}", s.len(), s).map_err(|_| ActorError::Internal)?; 

 Ok(()) 

 } 

 fn read_json_packet(&mut self) -> Result<Option<Value>, String> { 

 // https://firefox-source-docs.mozilla.org/devtools/backend/protocol.html#stream-transport 

 // In short, each JSON packet is [ascii length]:[JSON data of given length] 

 let mut buffer = vec![]; 

 loop { 

 let mut buf = [0]; 

 let byte = match self.read(&mut buf) { 

 Ok(0) => return Ok(None), // EOF 

 Err(e) if e.kind() == ErrorKind::ConnectionReset => return Ok(None), // EOF 

 Ok(1) => buf[0], 

 Ok(_) => unreachable!(), 

 Err(e) => return Err(e.to_string()), 

 }; 

 match byte { 

 b':' => { 

 let packet_len_str = match String::from_utf8(buffer) { 

 Ok(packet_len) => packet_len, 

 Err(_) => return Err("nonvalid UTF8 in packet length".to_owned()), 

 }; 

 let packet_len = match packet_len_str.parse::<u64>() { 

 Ok(packet_len) => packet_len, 

 Err(_) => return Err("packet length missing / not parsable".to_owned()), 

 }; 

 let mut packet = String::new(); 

 self.take(packet_len) 

 .read_to_string(&mut packet) 

 .map_err(|e| e.to_string())?; 

 debug!("{}", packet); 

 return match serde_json::from_str(&packet) { 

 Ok(json) => Ok(Some(json)), 

 Err(err) => Err(err.to_string()), 

 }; 

 }, 

                 c => buffer.push(c), 

 } 

 } 

 } 

 }

brentschroeter · 2026-04-07T19:31:59Z

You're absolutely right. I must have mixed up a merge or something. Thank you for catching it!

Do you want to make a PR to fix? Otherwise I can submit one later today.

This was supposed to be removed as part of servo#43472 see servo#43472 (comment) Signed-off-by: Bennet Bleßmann <bennet.blessmann+github@googlemail.com>

This was supposed to be removed as part of servo#43472 see servo#43472 (comment) and servo#43472 (comment) Signed-off-by: Bennet Bleßmann <bennet.blessmann+github@googlemail.com>

Skgland · 2026-04-07T20:57:36Z

Opened PR #44006

Removes the `impl JsonPacketStream for TcpStream` that was supposed to be removed as part of #43472 > Testing: [..] Removing the JsonPacketStream implementation for TcpStream should discourage regressions due to improper use of raw streams. Confirmed in #43472 (comment) Testing: no new test needed as this only removes code Signed-off-by: Bennet Bleßmann <bennet.blessmann+github@googlemail.com>

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 20, 2026

brentschroeter commented Mar 20, 2026

View reviewed changes

Comment thread components/devtools/actors/worker.rs

brentschroeter commented Mar 20, 2026

View reviewed changes

Comment thread components/devtools/lib.rs

eerii approved these changes Mar 24, 2026

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Mar 24, 2026

devtools: wrap TcpStream to synchronize network writes

292ab86

Signed-off-by: Brent Schroeter <contact@brentsch.com>

servo-highfive added the S-needs-rebase There are merge conflict errors. label Mar 25, 2026

brentschroeter and others added 4 commits March 25, 2026 16:11

unwrap lock results instead of expecting

a969588

Signed-off-by: Brent Schroeter <contact@brentsch.com>

simplify cloned variable names

d628ae3

Signed-off-by: Brent Schroeter <contact@brentsch.com>

replace unnecessary try/Ok() return pattern

b4dce2b

Co-authored-by: eri <eri@igalia.com> Signed-off-by: Brent Schroeter <contact@brentsch.com>

consolidate nested match expressions

44cf3e9

Signed-off-by: Brent Schroeter <contact@brentsch.com>

brentschroeter force-pushed the devtools-connection-type branch from abb7679 to 44cf3e9 Compare March 25, 2026 23:18

servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-rebase There are merge conflict errors. labels Mar 25, 2026

consolidate DevtoolsConnection doc comments

25cb5c4

Signed-off-by: Brent Schroeter <contact@brentsch.com>

brentschroeter force-pushed the devtools-connection-type branch from a18a595 to 25cb5c4 Compare March 26, 2026 05:58

eerii approved these changes Mar 26, 2026

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Mar 26, 2026

eerii added this pull request to the merge queue Mar 26, 2026

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 26, 2026

eerii mentioned this pull request Mar 26, 2026

devtools: Rename BrowsingContextActor variable names #43655

Merged

Merged via the queue into servo:main with commit c94722d Mar 26, 2026
30 checks passed

servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 26, 2026

Skgland mentioned this pull request Apr 7, 2026

devtools: remove impl JsonPacketStream for TcpStream #44006

Merged

Uh oh!

Conversation

brentschroeter commented Mar 20, 2026

Uh oh!

brentschroeter commented Mar 20, 2026

Uh oh!

brentschroeter commented Mar 20, 2026

Uh oh!

Uh oh!

Uh oh!

eerii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brentschroeter commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Skgland commented Apr 7, 2026

Uh oh!

brentschroeter commented Apr 7, 2026

Uh oh!

Skgland commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

brentschroeter commented Mar 25, 2026 •

edited

Loading