Adjust the logic added by #1246#1269
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1269 +/- ##
==========================================
- Coverage 86.64% 86.15% -0.49%
==========================================
Files 111 84 -27
Lines 11027 7771 -3256
==========================================
- Hits 9554 6695 -2859
+ Misses 1473 1076 -397 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| size = specific_buffer_size if specific_buffer_size is not None else receive_buffer_size | ||
| with sock_receive_lock: | ||
| received_bytes = sock.recv(size) | ||
| if not received_bytes: |
There was a problem hiding this comment.
This looks good to me, based on the docs of this recv the connection should retry before throwing an Error, and the behavior in _fetch_messages should properly handle the situation where the bytes are None or of length 0 🥇
But let me know if I miss understood the intent of the change
There was a problem hiding this comment.
Would disabling suppress_ragged_eofs when setting up the SSLContext help in this situation, allowing an exception to be caught?
I think the "unexpected EOF error" occurs when the underlying socket indicates EOF, but the connection was not shutdown at the SSL protocol level. Along with EOF from the other end of the connection, it can happen due to a proxy, or anything else on the network that can affect the connection.
Note that there may implications to disabling suppress_ragged_eofs, such as needing to catch SSLEOFError in other places...
(I'm not sure what the intention of this check was, so just throwing this out there... if removing the check "works", it's probably easier to just go that route...)
There was a problem hiding this comment.
Thanks for the input @eddyg 💯 I was able to find the source code of this behavior it does seem like enabling suppress_ragged_eofs could allow us to sort actual EOF from SSL_ERROR_EOF
Adding some sort of retry logic in _fetch_messages when a SSL_ERROR_EOF is raised could be a good idea, what do you think @seratch?
There was a problem hiding this comment.
Note that suppress_ragged_eofs is enabled by default in Python, and causes an improperly-terminated SSL session to just return "EOF".
By disabling it in the SSLContext, SSLEOFErrors would propagate out to this code. But as I mentioned, there may be consequences in doing so... there's likely a reason suppressing ragged EOFs defaults to True in Python.
It may be interesting to disable it in a test app and see if SSLEOFErrors are seen communicating with the Slack server pipeline (my understanding is that it is actually quite common to terminate a session without properly tearing down the SSL session inside... that's probably why this parameter defaults to True!)
Anyway, this suggestion may be nothing more than a red herring, because as I mentioned, I'm not sure why the two-lines being proposed for removal were added in the first place! (Apologies for adding confusion!)
There was a problem hiding this comment.
@eddyg @WilliamBergamin Thanks a lot for you both's comments here! I think it's fine to have this revert but we may add/change the internals more in the future for the betterment of EOF and other pattern handling!
Summary
This pull request tweaks an internal code in the built-in Socket Mode client. Since #1246 changes landed, we've received feedback from users that an active connection can be terminated unexpectedly.
I think that this specific change can be reverted for the sake of even more stable behavior.
Category (place an
xin each of the[ ])/docs-src(Documents, have you run./scripts/docs.sh?)/docs-src-v2(Documents, have you run./scripts/docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.