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

Turbo::Streams::BroadcastStreamJob raising ActiveJob::SerializationError with ActiveSupport::SafeBuffer #522

Closed
northeastprince opened this issue Nov 23, 2023 · 8 comments · Fixed by #683

Comments

@northeastprince
Copy link
Contributor

The callbacks defined using broadcasts_refreshes (from 2.0.0-beta.1) fail with

Failed enqueuing Turbo::Streams::BroadcastStreamJob to Async(default): ActiveJob::SerializationError (Unsupported argument type: ActiveSupport::SafeBuffer)
@northeastprince
Copy link
Contributor Author

Huh, for some reason the error's not showing up on my app anymore.

@northeastprince northeastprince closed this as not planned Won't fix, can't repro, duplicate, stale Nov 30, 2023
@janklimo
Copy link

The error comes from using Sidekiq with strong arguments. It's still present.

@seanpdoyle
Copy link
Contributor

The underlying issue here might've been resolved by rails/rails#50122. It's only 1 week old at this point (Nov 30, 2023), so depending on your version of Rails, that might not be a meaningful fix for some time.

With that fix in mind, I wonder if it'd be viable for the engine to conditionally package an ActiveJob::Serializers::ObjectSerializer extension to special case ActiveSupport::SafeBuffer instances when Rails.version <= 7.2.

@northeastprince
Copy link
Contributor Author

That must be why it was fixed for me! We're running on Rails edge. If there's a chance the Rails team will backport the fix, I could ask. If they probably won't, then I'll add the monkey-patch.

@northeastprince
Copy link
Contributor Author

Ah, I'm getting this error again when using Sidekiq.

northeastprince added a commit to hackclub/hackathons-backend that referenced this issue Dec 6, 2023
@northeastprince
Copy link
Contributor Author

northeastprince commented Dec 6, 2023

@seanpdoyle it looks like rails/rails#50122 didn't actually add a serializer for SafeBuffers and in Sidekiq their strict args checking (enabled by default) raises an error of the class itself isn't a string or a few other primitive types. I can open a PR but I can also see the Rails team not wanting a serializer for something that doesn't usually need to be serialized, in which case just calling to_str on the arg for the broadcast job might be the best option.

@janklimo
Copy link

janklimo commented Dec 7, 2023

It's possible I'm doing this wrong but adding the serializer didn't resolve it. Here's my initializer:

# frozen_string_literal: true

class SafeBufferSerializer < ActiveJob::Serializers::ObjectSerializer
  def serialize(argument)
    super({ 'value' => argument.to_s })
  end

  def deserialize(hash)
    ActiveSupport::SafeBuffer.new(hash['value'])
  end

  private

  def klass
    ActiveSupport::SafeBuffer
  end
end

ActiveJob::Serializers.add_serializers(SafeBufferSerializer)

And this still triggers the error:

Failed enqueuing Turbo::Streams::BroadcastStreamJob to Sidekiq(default): ArgumentError (Job arguments to Turbo::Streams::BroadcastStreamJob must be native JSON types, but "<turbo-stream action=\"refresh\"></turbo-stream>" is a ActiveSupport::SafeBuffer.
See https://github.com/sidekiq/sidekiq/wiki/Best-Practices
To disable this error, add `Sidekiq.strict_args!(false)` to your initializer.

Even if this somehow did work, it would invariably be slower than converting the ActiveSupport::SafeBuffer payload to a string in this gem before enqueueing the job so I'd vote for that instead.

Edit: the above wasn't working on 7.1.2. It does, however, work on Rails edge but to_s returns self, so you need to use argument.to_str or String.new(argument). Having said that, it's still a cleaner approach to let the gem make that conversion.

@Petercopter
Copy link

I used the workaround by @northeastprince

Sidekiq.strict_args!(false)

Just so I could play with this new toy. Wow! 🤯 I can't wait till this is production ready!

michaelbaudino added a commit to michaelbaudino/turbo-rails that referenced this issue Apr 26, 2024
This provides compatibility with Sidekiq as an ActiveJob adapter (because [Sidekiq only allows native JSON types][1] to be passed as job arguments).

All credit goes to [@jdelStrother] who [suggested this solution][2].

Fixes [hotwired#522] and [hotwired#535]

[1]: https://github.com/sidekiq/sidekiq/wiki/Best-Practices#1-make-your-job-parameters-small-and-simple
[2]: hotwired#535 (comment)
[@jdelStrother]: https://github.com/jdelStrother
[hotwired#522]: hotwired#522
[hotwired#535]: hotwired#535
AnalyzePlatypus added a commit to AnalyzePlatypus/turbo-rails that referenced this issue Sep 19, 2024
@dhh dhh closed this as completed in #683 Sep 19, 2024
dhh pushed a commit that referenced this issue Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment