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

Adding #hijacked? method to connection #30

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

yard
Copy link
Contributor

@yard yard commented Apr 4, 2024

A plain simple #hijacked? getter indicating hijack status of the connection.
This change's ultimate goal is become a part of socketry/async-http#151.

Types of Changes

  • New feature.

Contribution

@ioquatix
Copy link
Member

ioquatix commented Apr 4, 2024

def hijack! is the only place that @stream = nil.

Do you think we can use that as an indicator that the connection was hijacked?

e.g.

def hijacked?
  @stream.nil?
end

This would avoid introducing an extra instance variable.

@yard
Copy link
Contributor Author

yard commented Apr 5, 2024

tbh I have considered that but opted-in for an extra variable since subclasses can potentially break this by setting the @stream to nil without actually hijacking the connection (e.g. to make it crash later due to a stream error). This, however, is just me being extremely cautious since I'm not familiar enough with the rest of the ecosystem relying on this gem.

As long as you greenlight this, I am happy to revert to your solution :)

@ioquatix
Copy link
Member

ioquatix commented Apr 5, 2024

I understand your caution. I think this is okay, but if it turns out to be a bad idea, you can blame me :)

@yard
Copy link
Contributor Author

yard commented Apr 5, 2024

No worries, updated the PR :)

@ioquatix ioquatix merged commit 6d0b9ad into socketry:main Apr 5, 2024
21 of 22 checks passed
@ioquatix ioquatix added this to the v0.19.0 milestone Apr 5, 2024
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.

2 participants