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

💥 Changes to responses handling, motivated by thread-safety #93

Merged
merged 2 commits into from
Jan 7, 2023

Conversation

nevans
Copy link
Collaborator

@nevans nevans commented Jan 2, 2023

Calling #responses with no arguments or block is deprecated, as it
can't be made thread-safe and backward-compatible. For convenience, an
optional type paramater has been added to #responses, to yield only
the array for that type of response.

Added #clear_responses as a convenience method for the common "read
and delete" pattern, and to provide a thread-safe responses
method that doesn't require a block.

The response handlers methods are synchronized, but #response_handlers
now returns a frozen clone to alert users who had manipulated it
directly. Hopefully very few are affected, but this change is backwards
incompatible. N.b: there currently is no API for inserting a
response handler into a particular position in the array.

Also, improve yields_in_test_server_thread test helper: it no longer
reads last_tag from the block result, it sets a short IO#timeout in
ruby 3.2+, it uses a slightly longer timeout for overall test
completion, and it defines sock.getcmd so the tests can

@nevans nevans requested a review from shugo January 2, 2023 19:09
@nevans nevans changed the title Changes to responses handling, motivated by thread-safety 💥 Changes to responses handling, motivated by thread-safety Jan 2, 2023
Copy link
Member

@shugo shugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable.
However, I think old-style responses should be supported until the release of Ruby 3.3.

@nevans
Copy link
Collaborator Author

nevans commented Jan 5, 2023

I think old-style responses should be supported until the release of Ruby 3.3.

Agreed. IMO, our deprecation policy for anything non-trivial should be to warn for at least year after the first ruby release that includes the deprecation warning. So, if this deprecation warning is bundled with 3.2.1, the earliest we would break existing functionality would be one year after the 3.2.1 release (whenever that is). What do you think?

@shugo
Copy link
Member

shugo commented Jan 5, 2023

I think old-style responses should be supported until the release of Ruby 3.3.

Agreed. IMO, our deprecation policy for anything non-trivial should be to warn for at least year after the first ruby release that includes the deprecation warning. So, if this deprecation warning is bundled with 3.2.1, the earliest we would break existing functionality would be one year after the 3.2.1 release (whenever that is). What do you think?

It sounds fine!

There is no one single client_thread, this isn't used internally, and
it's misleading to external users.  However, adding `#reciever_thread`
(or `#receiver_fiber`) might be useful.
@nevans nevans force-pushed the response-handlers branch 2 times, most recently from 89ae479 to 223055e Compare January 6, 2023 22:39
Calling `#responses` with no block is deprecated, as it can't be made
thread-safe and backward-compatible.  However, the deprecation warning
is commented out for now, until after I can check the test suites for
the rails and mail gems and submit PRs there if necessary.

For convenience, an optional `type` paramater has been added to
`#responses`, to yield only the array for that type of response.

Added `#clear_responses` as a convenience method for the common "read
and delete" pattern, and to provide a thread-safe responses
method that doesn't require a block.

The response handlers methods are now synchronized, but
`#response_handlers` now returns a frozen clone to alert users who had
manipulated it directly.  Hopefully very few are affected, but this
change is backwards incompatible.  N.b: there currently is no API for
inserting a response_handler into a particular position in the array.

Also, improve the yields_in_test_server_thread test helper: it no longer
reads `last_tag` from the block result, it sets a short `IO#timeout` in
ruby 3.2+, it uses a slightly longer timeout for overall test
completion, and it defines `sock.getcmd` so the tests can read more
intuitively than using a proc passed into the block.
@nevans
Copy link
Collaborator Author

nevans commented Jan 7, 2023

@shugo For what it's worth, I've commented out the #responses deprecation warning for now. Before that is merged, I want to check the top four reverse dependencies (actionmailer, mail, actionmailbox, and gitlab-mail_room) and submit PRs to those gems, so they aren't forced to deal with deprecation warnings flooding their logs.

@nevans nevans merged commit 448d341 into master Jan 7, 2023
@nevans nevans deleted the response-handlers branch January 7, 2023 14:57
nevans added a commit that referenced this pull request Jan 7, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Jan 9, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Jun 12, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Aug 5, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Aug 5, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Sep 25, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Sep 26, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Oct 4, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Nov 4, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Nov 7, 2023
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Apr 30, 2024
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request May 19, 2024
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request May 19, 2024
This was extracted from #93 and split into a separate PR.

This allows us to prepare dependent projects for the new behavior before
their logs are flooded with deprecation warnings.
nevans added a commit that referenced this pull request Jun 12, 2024
This was extracted from #93 and split into a separate PR.

A new config option is added: `responses_without_block`.  This is
provided as a workaround, until dependant projects can update their
usage.  A future release may remove this backwards compatibility, but
_no sooner_ than v0.6.
nevans added a commit that referenced this pull request Jun 14, 2024
This was extracted from #93 and split into a separate PR.

A new config option is added: `responses_without_block`.  This is
provided as a workaround, until dependant projects can update their
usage.  A future release may remove this backwards compatibility, but
_no sooner_ than v0.6.
nevans added a commit that referenced this pull request Jun 15, 2024
This was extracted from #93 and split into a separate PR.

A new config option is added: `responses_without_block`.  This is
provided as a workaround, until dependant projects can update their
usage.  A future release may remove this backwards compatibility, but
_no sooner_ than v0.6.
nevans added a commit that referenced this pull request Jun 15, 2024
This was extracted from #93 and split into a separate PR.

A new config option is added: `responses_without_block`.  This is
provided as a workaround, until dependant projects can update their
usage.  A future release may remove this backwards compatibility, but
_no sooner_ than v0.6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants