setRegion in a threaded environment

[dnlbauer]

Im currently trying to rewrite my backend in java using Spring framework and I will serve something like 20 requests per second.

One problem I ran into is that im unsure how and if the static region field in Orianna will lead to concurrency problems. Think about the following scenario:

2 requests arrive simultaneously. One is for Region.EUW, the other for Region.NA. The 2 requests are handled at the same time in the same process (static variables are shared).

1. Req 1 sets Region to EUW
2. Req 2 sets Region to NA
3. Req 1 queries for Summoner -> fail

The request for the euw summoner will fail because request 2 set the region to something else before request 1 had time to make its api query-> request 1 queries the wrong endpoint.

Does anyone have an idea how to tackle this? Best regards, Danijoo

[davidherasp]

This should not happen since for every request a new instance of the application is created, isn't it? Thats how it works i think but I'm not comlpetly sure

El dom., 15 may. 2016 a las 12:04, danijoo (notifications@github.com) escribió:

Im currently trying to rewrite my backend in java using Spring framework and I will serve something like 20 requests per second.

One problem I ran into is that im unsure how and if the static region field in Orianna will lead to concurrency problems. Think about the following scenario:

2 requests arrive simultaneously. One is for Region.EUW, the other for Region.NA. The 2 requests are handled at the same time in the same process (static variables are shared).

1. Req 1 sets Region to EUW
2. Req 2 sets Region to NA
3. Req 1 queries for Summoner -> fail

The request for the euw summoner will fail because request 2 set the region to something else before request 1 had time to make its request -> request 1 queries the wrong endpoint.

Does anyone have an idea how to tackle this? Best regards, Danijoo

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/meraki-analytics/Orianna/issues/55

David H.

[dnlbauer]

I dont think this is how it works because things like Database Daos are shared across request in Spring framework.

[davidherasp]

What I do is create a Context Listener that when the context is created I create the DAO calling a method called init() that is going to initate it with the params I want (Oracle DB, MySQL, whatever you want) and then i set the DAO as a ServletContext Attribute so it's global not just a request thing

El dom., 15 may. 2016 a las 13:31, danijoo (notifications@github.com) escribió:

I dont think this is how it works because things like Database Daos are shared across request in Spring framework.

— You are receiving this because you commented.

Reply to this email directly or view it on GitHub https://github.com/meraki-analytics/Orianna/issues/55#issuecomment-219279937

David H.

[dnlbauer]

I just tested it with this code:

class TestClass {
    public static String foo = null;
}

then in my controller:

@RestController
public class MyController {

    @RequestMapping("/")
    public String get() {
      if(TestClass.foo == null) {
        TestClass.foo = this.toString();
        return "foo is null";
      } else return foo;
    }

}

if i now go to localhost:8080 it print "foo is null" for the first request. All future requests return MyController@ID where ID stays constant. So the static variable is shared across requests and would cause problems if its accessed from multiple requests simoultaneously. The same will happen with the static region variable in Orianna.

[davidherasp]

Yes but it just happens to you, not other users. For each user accessing your application all the context is new so other users Region won't affect at all

[jjmaldonis]

@danijoo This is something we are looking to fix during the next major rewrite/update of Orianna. While there are ways to get around it, the situation you posted in your first comment is an issue.

I'm not an expert with Ori but I have done quite a bit of work with its sister library Cassiopeia and they are implemented similarly. You might try this: Instantiate multiple instances of Orianna (which might require you to completely copy the entire library) for each region. Then use the appropriate copy for whichever region you created it for when requests are sent. Because your rate limit is only enforced per-region, you won't get rate limit errors. If you can get the requests to go to the right copy of the library, I think this could work.

[dnlbauer]

I'm not an expert with Ori but I have done quite a bit of work with its sister library Cassiopeia and they are implemented similarly. You might try this: Instantiate multiple instances of Orianna (which might require you to completely copy the entire library) for each region. Then use the appropriate copy for whichever region you created it for when requests are sent.

Thats not very convenient because I want to be able to load the library from jcenter instead of creating my own jars. Maybe ill create a fork were I address this issue on my own. I think there are two possible ways to tackle this:

1) make the region an instance field of RiotApi class. This way we can instantiate multiple Api objects with different settings and for different regions. 2) add the region as parameter to all calls. For example, RiotApi.getSummonerByName(name) would become RiotApi.getSummonerByName(name, region)

I tend to the first solution because it will give more control about the api to the dev using it. For example we can then also create different api instances with different caching mechanism and so on.

[dnlbauer]

Yes but it just happens to you, not other users. For each user accessing your application all the context is new so other users Region won't affect at all

@davidherasp no this is not true. static variables arent affected by context. A static variable remains the same for the whole process unless its loaded by a different class loader. create a static field and try it on your own. It is shared accross all requests because it lives in a static context.

[davidherasp]

@danijoo yeah sorry didn't know that region was static. And I agree with your approach of adding region as a parameter to calls. It feels more natural to me because it's all you need to make the request

[jjmaldonis]

@danijoo If you can create multiple instances of the RiotApi using Ori then there is no need to duplicate the entire library. In python there can only be one RiotApi object.

I would also lean towards creating multiple instances for now, partly because it will be fairly simple. When Rob does the rewrite (which might be quite a while from now, I am not sure) I know he definitely wants to fix this issue. If I remember correctly, I think we decided that we will implement your (2) by adding the region as a parameter to all calls. However, we will also be sure to add functionality to the methods that use the correct region automatically (e.g. if a MatchList is asked for through a Summoner, then we will use the same region for the MatchList call that the Summoner has) -- this will keep things almost as simple as they are now, but the whole library will be much more powerful.

There are a ton of other major updates that we plan to do that the same time, however, and it will essentially require a complete rewrite of the entire library, so it's a big project and there isn't a timeline for it yet.