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

Return nil on empty response #498

Merged
merged 1 commit into from
Oct 31, 2024
Merged

Conversation

vinistock
Copy link
Member

We are currently raising if something is printed to stdout from the server and it results in an empty message, but I'm not sure why we were raising in the first place.

This keeps filling up our telemetry with errors that aren't actionable. It could be that the application printed something, but since there's no content, I think it's okay to just return nil (since this method is already expected to be able to return nil).

@vinistock vinistock added the bugfix This PR fixes an existing bug label Oct 30, 2024
@vinistock vinistock self-assigned this Oct 30, 2024
@vinistock vinistock requested a review from a team as a code owner October 30, 2024 20:00
@vinistock vinistock requested a review from andyw8 October 30, 2024 20:01
@vinistock vinistock force-pushed the vs-return-nil-on-empty-response branch from 559ed6c to 62f3a45 Compare October 30, 2024 20:21
Copy link
Member Author

vinistock commented Oct 30, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @vinistock and the rest of your teammates on Graphite Graphite

@vinistock vinistock force-pushed the vs-return-nil-on-empty-response branch 7 times, most recently from 3f280d9 to 54081f0 Compare October 31, 2024 16:35
@vinistock vinistock force-pushed the vs-return-nil-on-empty-response branch from 54081f0 to 7caeea4 Compare October 31, 2024 16:51
@vinistock
Copy link
Member Author

For posterity, the issue with Windows was that we were only turning on sync for the STD pipes on the server side, but not on the client side, which may result in the OS deciding to buffer and then the client never receives responses back.

@vinistock vinistock merged commit 90f8a47 into main Oct 31, 2024
29 checks passed
@vinistock vinistock deleted the vs-return-nil-on-empty-response branch October 31, 2024 17:10
vinistock added a commit that referenced this pull request Nov 6, 2024
We're sometimes hitting `EmptyMessageError` in certain cases after the changes in #498. We only rescue `IncompleteMessageError`, which then makes it look like the add-on is crashing.

Let's group the two errors under a common ancestor, so that we can rescue all types of message errors where needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants