8000 fix: throw exception on stream_socket_enable_crypto() warning by ramunasd · Pull Request #1158 · php-amqplib/php-amqplib · GitHub
[go: up one dir, main page]
Skip to content

fix: throw exception on stream_socket_enable_crypto() warning #1158

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 7, 2024
Merged

fix: throw exception on stream_socket_enable_crypto() warning #1158

merged 1 commit into from
Feb 7, 2024

Conversation

ramunasd
Copy link
Member
@ramunasd ramunasd commented Feb 6, 2024

This fixes 2 issues introduced in version 3.6.0:

  • all warnings from function stream_socket_enable_crypto() are ignored and it's not good, because they provide valueable info about wrong SSL options or peer validation issues. Any consequent call to that method makes no sense since handshake is failed and connection is not useable at all.
  • no proper check for function return value - according to docs, function returns 0 if there isn't enough data and you should try again, false if negotiation has failed, but that is not checked and loop continues without any delay until timeout is reached

@ramunasd ramunasd added the bug label Feb 6, 2024
@ramunasd ramunasd added this to the 3.6.1 milestone Feb 6, 2024
@ramunasd ramunasd requested a review from lukebakken February 6, 2024 20:12
@ramunasd ramunasd force-pushed the throw_crypto_method_warning branch from 2e18096 to 64f776c Compare February 6, 2024 20:15
@ramunasd
Copy link
Member Author
ramunasd commented Feb 6, 2024

@egorgrushko please review this fix

@codecov Codecov
Copy link
codecov bot commented Feb 6, 2024 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (109b13d) 74.10% compared to head (e344ed4) 74.58%.
Report is 1 commits behind head on master.

Files Patch % Lines
PhpAmqpLib/Wire/IO/StreamIO.php 84.61% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #1158      +/-   ##
============================================
+ Coverage     74.10%   74.58%   +0.47%     
- Complexity     1042     1044       +2     
============================================
  Files            39       39              
  Lines          3136     3144       +8     
============================================
+ Hits           2324     2345      +21     
+ Misses          812      799      -13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@egorgrushko
Copy link
Contributor

@ramunasd I'm concerned about usleep.
Let's say you have a web application that produces messages (not a worker). As far as I understand, due to the PHP nature, connection will be established on every HTTP request, so that usleep will increase response time for every request by at least 10ms, which is a lot.

@ramunasd ramunasd force-pushed the throw_crypto_method_warning branch from 64f776c to e344ed4 Compare February 7, 2024 06:52
@ramunasd
Copy link
Member Author
ramunasd commented Feb 7, 2024

@egorgrushko I added instant return if enable is successful on first attempt and lower delay, just 1ms. In the future all connections will be lazy, so this won't affect applications so much.

@lukebakken lukebakken merged commit 76eee28 into master Feb 7, 2024
@lukebakken lukebakken deleted the throw_crypto_method_warning branch February 7, 2024 17:21
@ramunasd
Copy link
Member Author
ramunasd commented Feb 9, 2024
8000

I will release this as patch version 3.6.1 on Monday.

kratkyzobak pushed a commit to kratkyzobak/php-amqplib that referenced this pull request Feb 9, 2024
@ramunasd
fix: throw exception on stream_socket_enable_crypto() warning (php-am…
8bcd6cb 
…qplib#1158)
@lukebakken
Copy link
Collaborator

@ramunasd thank you!

ramunasd added a commit that referenced this pull request Feb 12, 2024
@ramunasd
fix: throw exception on stream_socket_enable_crypto() warning (#1158)
2a33e45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukebakken lukebakken lukebakken approved these changes

Assignees

@ramunasd ramunasd

@lukebakken lukebakken

Labels
bug
Projects
None yet
Milestone
3.6.1
Development

Successfully merging this pull request may close these issues.
3 participants
@ramunasd @egorgrushko @lukebakken
0