SketchUp / api-issue-tracker

Public issue tracker for the SketchUp and LayOut's APIs
https://developer.sketchup.com/
39 stars 10 forks source link

What's wrong with the StdLib HTTP module #110

Open Eneroth3 opened 6 years ago

Eneroth3 commented 6 years ago

In SketchUp 2017 a Sketchup::Http [sic] module was added with this explanation:

This is an alternative to the Net::Http module that comes with Ruby StdLib - which is known to have issues within SketchUp.

It would be very helpful if the docs could state what the issues with the StdLib were.

Reference to forum thread where the question has been answered by Dan: https://forums.sketchup.com/t/stdlib-http-in-sketchup/74354/4.

DanRathbun commented 6 years ago

There is also mismatch between the method names in the new Sketchup::Http module, and the Ruby Standard Library Net::HTTP classes.

Sketchup::Http::Request Net::HTTPGenericRequest Notes
#cancel() none No equivalent in std lib.
#headers() #to_hash()
#headers()= hash #initialize_http_header(hash)
#url() #uri().to_s() see note below
#body= URI.encode_www_form(hash) #set_form_data(hash) "require 'uri'"

Sketchup::Http::Request lacks many of the nifty methods that the Net::HTTPHeader mixin module provides.

URI class use by itself needs a "require 'uri'" statement. If using standard libraries a "require 'net/http'" call will in turn load the URI class as the HTTP classes have methods that both return URI objects or expect them as method arguments.


Sketchup::Http::Response Net::HTTPResponse Notes
#headers() #to_hash() #to_hash may also convert fields to arrays
#status_code() #code().to_i()
#status_code().to_s() #code() RFC2616 response codes
none #uri() #uri() actually returns a URI object

Sketchup::Http::Response lacks all of the rest of the attributes and methods that Net::HTTPResponse has, including the nifty methods that the Net::HTTPHeader mixin module provides.

The #status_code / #code methods need to be compared against RFC2616 server response codes:

The Sketchup::Http constants listed are special integer codes for use ONLY with the internal status of the Sketchup::Http::Request#status() method. Ie ... this will fail (because I tried it with false assumptions) ...

if @response.status_code == Sketchup::Http::STATUS_SUCCESS
  # do happy success code
else

You need to do like ...

if @response.status_code.between?(200,299)
  # do happy success code
else

SEE ALSO ISSUES:

DanRathbun commented 6 years ago

Okay I found another very annoying quirk with the SketchUp API classes.

The standard library the Net::HTTPHeader mixin module creates header access methods with case-insensitive key arguments. I got spoiled using them.

The plain jane hashes returned by the SU API classes' #headers methods are case sensitive. This caused hours of frustrating hard to track down errors. Luckily I'm only dealing with a few servers, so I can examine the actual response headers and adapt to the exact caseness of the keys.

kengey commented 5 years ago

The stdlib module depends on openssl. The version of openssl included in Ruby 2.2 had a tremendous bug on Windows. Also see this: https://stackoverflow.com/questions/29984838/openssl-causing-very-slow-rails-boot-time-on-windows and https://bugs.ruby-lang.org/issues/12139

TLDR. In order to do random number generation, upon initialization, openssl traverses the entire RAM used by the process. So, if a user launches SU with a large model, this causes Sketchup to freeze for several minutes.

Many things are affected by openssl, like, securerandom for example. I ran into this issue with a client of mine where we did cryptography. I solved it temporarely by overwriting openssl.so by the one shipped with ruby 2.3

MSP-Greg commented 5 years ago

The version of openssl included in Ruby 2.2

Re the ruby issue mentioned, it was backported and appeared in Ruby 2.2.5. As I recall, there were still 'first use' time issues.

overwriting openssl.so by the one shipped with ruby 2.3

Not sure, but I think that would be linked to x64-msvcrt-ruby230.dll. Did you edit it, or maybe use openssl.so from the RubyInstaller Ruby 2.2.6 build?

Regardless, all 'startup time' issues in OpenSSL on Windows were fixed in OpenSSL 1.1.0. Currently, Ruby Windows versions from 2.5 and later are using OpenSSL 1.1.1 (with TLSv1.3).

I never tried SU with Ruby 2.2.6. It might just drop right in...

I think SU 2017 and later ship with 2.2.4?

kengey commented 5 years ago

Not sure, but I think that would be linked to x64-msvcrt-ruby230.dll

Good remark. I am sure I replaced it with the v2.3 variant. I never did much testing to see if it breaks other stuff, but the specific issue got resolved by the replacement. However, since we were not willing to take the risk of breaking other things, we ended up changing the user's workflow so that he boots Sketchup with an emtpy model, intitialize the plugin en then load the model he intended to open.

thomthom commented 5 years ago

Yea, as Greg said, it'd be highly surprising if a .so file for another Ruby major/minor version loaded. Because of the linking it shouldn't work.

SketchUp 2017 - 2018 ship with 2.2.4.

I added a cop for it in our RuboCop extension: https://rubocop-sketchup.readthedocs.io/en/latest/cops_performance/#sketchupperformanceopenssl

But I'm thinking it would be worth adding more details to provide more context.

kengey commented 5 years ago

Yea, as Greg said, it'd be highly surprising if a .so file for another Ruby major/minor version loaded. Because of the linking it shouldn't work.

SketchUp 2017 - 2018 ship with 2.2.4.

I added a cop for it in our RuboCop extension: https://rubocop-sketchup.readthedocs.io/en/latest/cops_performance/#sketchupperformanceopenssl

But I'm thinking it would be worth adding more details to provide more context.

Hmm, it looks I'm wrong. I tried it again to require openssl with the v2.3 version, and, exactly as you stated, that doesn't work. I guess what happened was: I replaced openssl.so, but only required securerandom in my tests. For some reason that loads without openssl.so existing and I generated an uuid using SecureRandom.uuid that didn't take ages to generate when a large model was opened.

MSP-Greg commented 5 years ago

For some reason that loads without openssl.so existing

https://github.com/ruby/ruby/blob/v2_2_10/lib/securerandom.rb

On Windows, it appears that SecureRandom uses a Windows API call to CryptGenRandom, regardless of whether OpenSSL loads (checked Ruby 2.2.10 only).

Never knew that. Good fiddle example.

Regardless, Thomas identified the Windows OpenSSL 1.0.1/1.0.2 issue a long time ago. My testing with SU 2017 showed that it just affected the 'init' or 'first use' of Ruby OpenSSL functions. I threw Ruby 2.3 with OpenSSL 1.1.0 in SU 2017, and the problem disappeared...

kengey commented 5 years ago

My testing with SU 2017 showed that it just affected the 'init' or 'first use' of Ruby OpenSSL functions.

Yes, that is why I ended up advising the client to launch SketchUp without a model and thus allowing the plugin to initialize in a less RAM consuming process.

thomthom commented 5 years ago

For some reason that loads without openssl.so existing and I generated an uuid using SecureRandom.uuid that didn't take ages to generate when a large model was opened.

It actually mutes loading errors of OpenSSL....

https://github.com/ruby/ruby/blob/933bb2b8b57c48dc664fce6d752670e6521a4141/lib/securerandom.rb#L2-L5

begin
  require 'openssl'
rescue LoadError
end

So you inadvertently avoided the OpenSSL penalty because you broke OpenSSL in your installation. And because they muted the loading errors it looked like everything was shiny.

On Windows, it appears that SecureRandom uses a Windows API call to CryptGenRandom, regardless of whether OpenSSL loads (checked Ruby 2.2.10 only).

Oh, that's interesting! Wasn't aware of it. Looks like it did this for the 2.0 releases as well: https://github.com/ruby/ruby/blob/v2_0_0_648/lib/securerandom.rb

But since it tries to load OpenSSL it still takes the performance penalty unfortunately.

https://github.com/ruby/ruby/blob/v2_2_10/lib/securerandom.rb#L113

honkinberry commented 2 years ago

This one really aggravates me, if I may be so blunt. Especially given that Sketchup::Http has no synchronous option, and definitions.load_from_url can't be called from an HtmlDialog (not going to get into a debate here over the failings of using the UI.start_timer workaround, can we just agree that it is broken?). My fallback to these other issues would be to use Net::HTTP, but that fails miserably. Here's where I get really peeved about this -- this has been an issue since 2017 at least, if not earlier. Add to that, even my stupid brain shows that it is not that difficult to fix, as it sure seems to point out exactly where the error is occurring:

url = "https://www.yahoo.com/" uri = URI(url) # change it to a URI construct http = Net::HTTP.new(uri.host, uri.port) request = Net::HTTP::Get.new(uri.request_uri) response = http.request(request) if ( response.is_a?(Net::HTTPSuccess) ) puts response.body end # if

Error: # /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/protocol.rb:225:in rbuf_fill' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/protocol.rb:191:inreaduntil' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/protocol.rb:201:in readline' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/http/response.rb:42:inread_status_line' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/http/response.rb:31:in read_new' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/http.rb:1528:inblock in transport_request' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/http.rb:1519:in catch' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/http.rb:1519:intransport_request' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/http.rb:1492:in request' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/http.rb:1485:inblock in request' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/http.rb:933:in start' /Applications/SketchUp.app/Contents/Frameworks/Ruby.framework/Versions/2.7.1/lib/ruby/2.7.0/net/http.rb:1483:inrequest'

:4:in `
' SketchUp:in `eval'
MSP-Greg commented 2 years ago

@honkinberry

Maybe something like:

# requires as needed
require 'net/http'
require 'uri'
require 'openssl'

def get_body(url)
  body = nil
  uri = URI url

  Net::HTTP.start(uri.host, uri.port, :use_ssl => (uri.scheme == 'https'))  do |http|
    req = Net::HTTP::Get.new uri.request_uri
    resp = http.request req
    case resp
    when Net::HTTPSuccess
      body = resp.body
    when Net::HTTPRedirection
      puts "#{resp.class}\n   Redirect: #{resp['location']}"
      body = get_body resp['location']
    else
      puts resp.class
    end
  end
  body
end
honkinberry commented 2 years ago

Well slap my fanny and call me daddy, that works. @MSP-Greg, you are a hero among mere mortals.

Alright, now I can rip out all the nonsense code attempting to use the utterly pointless Sketchup::Http. Back in business!!! Thank you.

MSP-Greg commented 2 years ago

Glad I could help.

I've been coding for a long time, and (years ago) I was introduced to Ruby via SketchUp. Now, I work a lot more with standalone Ruby than Ruby with SketchUp. Sketchup::Http can be helpful, but sometimes Net::HTTP may work better.

Note that the above code is for reasonably sized response bodies. If one is downloading large files, it could be optimized for that. Might use less memory, but speed wouldn't be affected much...