FIX: default with_session client options

[rreckonerr]

The default HTTPClientOptions.new() parameter in with_session method raises an error. According to the README it should be possible to call with_session without passing any arguments.

[JanEbbing]

Thanks for your work! This looks good to me overall, just 2 small nits.
I will also look to add a test case that uses with_session to make 2 requests in a VCR on top of your changes.

[JanEbbing]

Nit 1: please add this copyright header for new files

# Copyright 2024 DeepL SE (https://www.deepl.com)
# Use of this source code is governed by an MIT
# license that can be found in the LICENSE file.

[JanEbbing]

Nit 2: IMO this file should go under spec/http_client_options_spec.rb - integration_tests is reserved for files that make actual web requests to the API, and the spec/ folder in general emulates the structure of the source code in lib/.

[JanEbbing]

Thanks for adding tests :)

[rreckonerr]

My pleasure!

[rreckonerr]

@JanEbbing Thanks for quickly checking it out!

I have a question, maybe you have an idea of what's going on.
I used Concurrent::Promises before to make translation requests in parallel, but upgrading from 2.5.3 to 3.0.1 caused weird errors (like FrozenError can't modify frozen OpenSSL::SSL::SSLContext), so I refactored away to use the with_session. Do you think that it might indicate some thread-safety issues and whether there is a way to make the requests in parallel? I didn't manage to reproduce the issue in tests, unfortunately.

Below you can see the previous Concurrent::Promises usage

@@ -19,8 +19,8 @@ def translate_single(text, source_lang, target_lang, options = DEFAULT_OPTIONS)
   end
   
   def translate_multiple(text, source_lang, target_langs, options = DEFAULT_OPTIONS)
-    promises = target_langs.map do |target_lang|
-      Concurrent::Promises.future do
+    with_session(DeepL::HTTPClientOptions.new({}, nil)) do |_session|
+      target_langs.to_h do |target_lang|
         translation = translate(text, local_to_external(source_lang), local_to_external(target_lang), options.to_h.deep_dup)
         [external_to_local(target_lang), translation.text]
       rescue DeepL::Exceptions::Error => e
@@ -28,10 +28,6 @@ def translate_multiple(text, source_lang, target_langs, options = DEFAULT_OPTION
         [external_to_local(target_lang), nil]
       end
     end
-
-    results = Concurrent::Promises.zip(*promises).value!
-
-    results.to_h
   end

[JanEbbing]

I'll look into this tomorrow - I did slightly change how the requests are made internally, but I dont think it should cause issues for this use case.

[JanEbbing]

Hi, I cannot reproduce any issues with the following code:

require 'concurrent-ruby'
require 'deepl'


def translate_multiple(text, source_lang, target_langs)
  promises = target_langs.map do |target_lang|
    DeepL.with_session(DeepL::HTTPClientOptions.new({}, nil)) do |_session|
      Concurrent::Promises.future do
        DeepL.translate(text, source_lang, target_lang)
        rescue DeepL::Exceptions::Error => e
          puts e.message
      end
    end
  end
  return Concurrent::Promises.zip(*promises).value!
end

sample_text = 'I am an example sentence in english.' * 1000
target_langs = ['DE', 'ES', 'PT-BR', 'IT', 'ZH-HANS', 'KO']
res = translate_multiple(sample_text, 'EN', target_langs)
puts res

But I need to read a bit more to exclude any thread safety issues, as the with_session will use a Net::HTTP session now - to fix this, we might need to add a different HTTP client as a dependency.

[rreckonerr]

This looks related getsentry/sentry-ruby#1696

For me the error pops up in production reliably, maybe it's related to yjit, which is enabled only in prod in my case, e.g.

RUN apt-get -qq install -y --no-install-recommends libjemalloc2

ENV LD_PRELOAD="libjemalloc.so.2"
ENV MALLOC_CONF="dirty_decay_ms:1000,narenas:2,background_thread:true"
ENV RUBY_YJIT_ENABLE="1"

[lasseebert]

I have the same FrozenError when running concurrent threads (with GoodJob in my case) that translate via DeepL. It seems to be because @http_client is reused between requests. Using with_session in each thread fixes it.