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

Try recv timeout #284

Merged
merged 10 commits into from Nov 18, 2021
Merged

Try recv timeout #284

merged 10 commits into from Nov 18, 2021

Conversation

Xaeroxe
Copy link
Contributor

@Xaeroxe Xaeroxe commented Nov 17, 2021

There are currently two prominent ways to receive messages on the ipc channel, you can either block indefinitely, or not at all. For my use case, I'd like to have the lower CPU usage that comes with blocking, but also be able to exit the call periodically so my thread can check if it needs to exit. So I've added the try_recv_timeout method, and implemented it for all supported platforms. This method uses the platform facilities to block a recv call, but only for a set duration, at which point it will return TryRecvError::Empty if nothing came back.

@highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Nov 17, 2021

Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thank you!

let mut overlapped: winapi::um::minwinbase::OVERLAPPED = mem::zeroed();
// Create a manually reset event. The documentation for GetOverlappedResultEx
// states you must do this in the remarks section.
overlapped.hEvent = CreateEventA(ptr::null_mut(), winapi::shared::minwindef::TRUE, winapi::shared::minwindef::FALSE, ptr::null_mut());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Let's import winapi::shared::minwindef::TRUE and winapi::shared::minwindef::FALSE to make this easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in bec4ef6

},
BlockingMode::Timeout(duration) => {
let events = libc::POLLIN | libc::POLLPRI | libc::POLLRDHUP;
let fd = &mut [libc::pollfd {fd, events, revents: 0}];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's write this as let mut fd = [...] instead. I find that easier to reason about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in da99fca

@@ -612,7 +612,8 @@ impl OsIpcOneShotServer {
let socket_path = temp_dir.path().join("socket");
let path_string = socket_path.to_str().unwrap();

let (sockaddr, len) = new_sockaddr_un(CString::new(path_string).unwrap().as_ptr());
let path_c_string = CString::new(path_string).unwrap();
let (sockaddr, len) = new_sockaddr_un(path_c_string.as_ptr());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Thank you for finding and fixing this.

@jdm
Copy link
Member

jdm commented Nov 18, 2021

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit da99fca has been approved by jdm

@highfive highfive assigned jdm and unassigned emilio Nov 18, 2021
@bors-servo
Copy link
Contributor

⌛ Testing commit da99fca with merge a57b51e...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis, status-appveyor
Approved by: jdm
Pushing a57b51e to master...

@bors-servo bors-servo merged commit a57b51e into servo:master Nov 18, 2021
@Xaeroxe Xaeroxe deleted the try-recv-timeout branch November 18, 2021 02:42
bors-servo added a commit that referenced this pull request Nov 18, 2021
fix my memory leak from PR 284

I realized after my PR #284 was merged that I had accidentally created a memory leak. This fixes it.

@jdm
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.

None yet

5 participants