-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Revert "Fix #4561 (revert #4430)" -- i.e. go back to modified #4430. #4568
Conversation
note: technically reactor users can still encounter a problem (if they enable blockhound) but now the error will be shown, which is easier to know how to respond (blockhound allowlist) oh i missed that the release notes doesn't say reactor issues are fixed, you can disregard^ |
@cowtowncoder done! Currently my workaround comment uses Should the workaround instead allowlist |
I don't think we should worry too much about this. Let reactor-core blockhound users do their own testing. The key thing is that we fix the code so that we don't try to release a lock that we never acquired - then the blockhound users will get the right exception message and they can worry about how to fix it. |
+1 to @pjfanning but also because we are unlikely to change the structure or naming for 2.x. @iProdigy You are right to be wary of internal methods, naming (we do change them over time). But in this it probably as the best choice. As aside, Ah yes: removal is wrt #1917 will file an issue for adding deprecation in 2.18. |
Reverts #4562, as per discussion #4430 the fix we have (minor change to lock scoping) that confirms fix seems safe. So we should be good to go in patch (2.17.2), not waiting for next minor release (2.18.0).
Apologies for the whiplash to anyone following discussions. :)