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

Refactor #system_connect without yield #14383

Conversation

straight-shoota
Copy link
Member

Socket#system_connect yields in case of an error. I understand the intention to avoid actually raising an exception for efficient error handling in case multiple targets are being tried (for example with Addrinfo.resolve).
But I’m not sure what’s the motivation for yielding instead of returning the Exception. The yield always happens just right before the method returns. This is a simple refactor of an internal method to make it simpler and easier to reason about.

The public method Socket#connect keeps the yielding behaviour to not break the API.

This cleans up the internal interface a in preparation of a bigger refactoring of the event loop (#10766).

@ysbaddaden
Copy link
Contributor

Yes, the initial idea (#3750) was to avoid raising/catching an exception when trying multiple IP to connect to. I suppose the yield crept up to the internal calls.

@straight-shoota
Copy link
Member Author

I suppose yielding could make some sense if the exception was allocated on the stack to avoid heap allocations. Then a caller who wants to raise, would need to clone it to the heap first. That seems a bit brittle, though.

@straight-shoota straight-shoota added this to the 1.12.0 milestone Mar 22, 2024
@straight-shoota straight-shoota merged commit 2f82786 into crystal-lang:master Mar 24, 2024
57 of 58 checks passed
@straight-shoota straight-shoota deleted the refactor/socket-connect-return-error branch March 24, 2024 14:46
@straight-shoota
Copy link
Member Author

FTR: I was wondering whether the yield setup could have any effect on the strack trace. But it seems it does not. The strack trace is exactly the same with or without this change (except for shifted line numbers of course).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants