krtschmr / monero

Ruby client to connect to the Monero RPC
MIT License
24 stars 12 forks source link

Check thread safety #15

Closed xaviablaza closed 5 years ago

xaviablaza commented 5 years ago

Description

Currently, global variables are set in the configuration of the remote procedure call (RPC). When using RPC, we may run into a race condition if the value of the global variable is written to in a multi-thread environment. We need to have a way to set up the configuration of the RPC client such that the global variable isn't changed, so that RPC calls are thread-safe.

Tasks

  1. See how the ethereum.rb HttpClient works.
  2. Couple the functionality of the RPC::Config and RPC::Client
krtschmr commented 5 years ago

can you reproduce a quick environment where we can see that issue?

so instead of @@var we need switch to @var ? do you have a suggestion how we can face those issues?

xaviablaza commented 5 years ago

can you reproduce a quick environment where we can see that issue?

I'm writing a PR using this gem towards a cold storage solution: bloom-solutions/crypto-cold-store

so instead of @@var we need switch to @var ? do you have a suggestion how we can face those issues?

Yes, we should use instance variables and couple the RPC::Config into the RPC::Client. Currently, you would need to do this to initialize the client:

RPC.config.host = "monerohost.com"
RPC.config.port = 38083
RPC.config.username = "username"
RPC.config.password = "password"
RPC.config.transfer_clazz = "MoneroTransfer"
monero_client = RPC

I propose to remove RPC::Config and do the following:

monero_client = RPC::Client.new(host, port, username, password, transfer_clazz)

Then within RPC::Client

module RPC
  class Client
    attr_reader :host, :port, :username, :password, :debug, :transfer_clazz

    def initialize(host, port, username, password, debug, transfer_clazz)
      @host = host
      @port= port
      @username= username
      @password= password
      @debug= debug
      @transfer_clazz= transfer_clazz
    end

    [...]
  end
end

I can write a PR for this change and you can review?

krtschmr commented 5 years ago

yes please write a PR for this. i won't merge it immediately but soon.

i wonder if this is actually a good change, since it will break any application with the current usage.

is there a way we can have it implemented both ways simultaneously so we can then drop the old style in 6 months? at least we should have a deprecation warning, ya?

xaviablaza commented 5 years ago

is there a way we can have it implemented both ways simultaneously so we can then drop the old style in 6 months? at least we should have a deprecation warning, ya?

Okay sounds good, I can do that.