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

InfluxDBResultMapper not thread safe? #430

Closed
Schntzlbrmpf opened this issue Mar 16, 2018 · 4 comments
Closed

InfluxDBResultMapper not thread safe? #430

Schntzlbrmpf opened this issue Mar 16, 2018 · 4 comments

Comments

@Schntzlbrmpf
Copy link

I am refering to this part of the documentation:

InfluxDBResultMapper resultMapper = new InfluxDBResultMapper(); // thread-safe - can be reused
List cpuList = resultMapper.toPOJO(queryResult, Cpu.class);

The problem is when I perform a multi-threaded mapping similar to this part of the code in several threads in parallel:

List cpuList = resultMapper.toPOJO(queryResult, Cpu.class);

I do get the following exception:

org.influxdb.InfluxDBMapperException: java.lang.IllegalAccessException: Class org.influxdb.impl.InfluxDBResultMapper can not access a member of class org.influx.test.Cpu with modifiers "private"
at org.influxdb.impl.InfluxDBResultMapper.parseSeriesAs(InfluxDBResultMapper.java:184)
at org.influxdb.impl.InfluxDBResultMapper.lambda$null$2(InfluxDBResultMapper.java:100)

The problem is most likely that the first thread did already set the accessibility of a field back to its original value when the second thread tries to write the field.

@fmachado
Copy link
Contributor

:-O

I'll fix the IllegalAccessException caused by the concurrent modification on the 'accessible' flag.

I'm sure we may see even performance improvements by checking first if the member is accessible or not, before setting it to true. Currently we are wasting time here because setAccessible is invoking System.getSecurityManager() every time.

Currently I'm returning the 'accessible' flag to the previous state but I don't think there is any benefit from doing it so I'll remove this approach.

Thanks @Schntzlbrmpf !

@Schntzlbrmpf
Copy link
Author

Hi Fernando,

The way I see it, it might be the best solution to:

  1. either allow the @column annotation on getters and setters as well which by definition must be public
  2. or have the annotation processor check if there are getters and setters on the POJO and use them instead of the Field - here you could fail if there are no such methods

The main problem with using the field directly is that it must be public if we want a fast mapping. If we could make the mapper call the (public) accessor methods instead, we might get a considerable performance increase and at the same time keep a clean interface even to our POJOs.

Best regards and thank you,
Andreas.

@fmachado
Copy link
Contributor

@Schntzlbrmpf The current implementation is making changes (by setting the accessible flag to true) that were supposed to be permanent so, there is no reason to revert it. I did this initially because (1) I could not identify any collateral affect and (2) it's more simple and performant than both solutions that you proposed (remember: I'm doing not per instance but at class level).

Take a look at the PR that I've just added here. Am I missing something?

@Schntzlbrmpf
Copy link
Author

Hello Fernando,

Sorry for this unbelievably late response. I think this is an acceptable solution. Thank you for your quick fix.

Best regards ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants