BingAds / BingAds-Java-SDK

Other
43 stars 48 forks source link

ConcurrentModificationException at apache-cxf #184

Closed schabe77 closed 6 months ago

schabe77 commented 6 months ago

After switching to 13.0.20 I sometimes get this the exception

java.util.ConcurrentModificationException: null
    at java.base/java.util.ArrayList.sort(ArrayList.java:1806)
    at org.apache.cxf.jaxrs.provider.ProviderFactory.sortReaders(ProviderFactory.java:740)
    at org.apache.cxf.jaxrs.provider.ProviderFactory.setCommonProviders(ProviderFactory.java:668)
    at org.apache.cxf.jaxrs.client.ClientProviderFactory.setProviders(ClientProviderFactory.java:73)
    at org.apache.cxf.jaxrs.provider.ProviderFactory.setUserProviders(ProviderFactory.java:868)
    at org.apache.cxf.jaxrs.client.spec.ClientImpl$WebTargetImpl.request(ClientImpl.java:282)
    at com.microsoft.bingads.internal.restful.RestfulServiceClient.createRequestBuilder(RestfulServiceClient.java:126)
    at com.microsoft.bingads.internal.restful.RestfulServiceClient.processRequestAsync(RestfulServiceClient.java:336)
    at com.microsoft.bingads.internal.restful.BulkService.sendRequestAsync(BulkService.java:83)
    at com.microsoft.bingads.internal.restful.BulkService.uploadEntityRecordsAsync(BulkService.java:202)
    at com.microsoft.bingads.v13.bulk.BulkServiceManager.uploadEntityRecordsImpl(BulkServiceManager.java:196)
    at com.microsoft.bingads.v13.bulk.BulkServiceManager.uploadEntitiesAsync(BulkServiceManager.java:181)

it seems the ProviderFactory isn't thread safe. would it be possible to put the access to WebTarget within a synchronized block?

schabe77 commented 6 months ago

I fixed it by replacing the rather poor cxf library with jersey client.

dennis-yemelyanov commented 6 months ago

thank you for reporting this! Yes, looks like CXF is not thread safe. A fix should be released soon

schabe77 commented 6 months ago

Thank you. I changed my dependencies this way and it seems to work:

        <dependency>
            <groupId>com.microsoft.bingads</groupId>
            <artifactId>microsoft.bingads</artifactId>
            <version>13.0.20</version>
            <exclusions>
                <exclusion>
                    <artifactId>cxf-rt-rs-client</artifactId>
                    <groupId>org.apache.cxf</groupId>
                </exclusion>
            </exclusions>
        </dependency>
        <dependency>
            <groupId>org.glassfish.jersey.core</groupId>
            <artifactId>jersey-client</artifactId>
        </dependency>

With jersey I don't have these multithreading problems. I'm not sure if it's a good idea to synchronize the access at an unknown amount of positions because a rs-implementation is poorly implemented.

dennis-yemelyanov commented 6 months ago

Yeah the standard is also not clear on what should be thread safe or not, looks like there is an open issue regarding this: https://github.com/jakartaee/rest/issues/494

I'm not sure if other implementations are thread safe, so it's probably better to add synchronization around the request() call as you suggested. 13.0.20.1 has a fix for this.

By default we use CXF since it usually has better performance, but it's true that sometimes we may run into tricky issues like this.

By the way, when using a different RS implementation, we recommend to enable message compression for better performance. For example, this works for Jersey and 13.0.20.1:

GlobalSettings.setHttpClientProvider(new HttpClientProvider() 
{ 
    @Override 
    protected ClientBuilder configureClientBuilder(ClientBuilder clientBuilder) { 
        return super.configureClientBuilder(clientBuilder)
            .register(org.glassfish.jersey.message.GZipEncoder.class)
            .register(org.glassfish.jersey.client.filter.EncodingFilter.class);
    }
});
dmitchell commented 1 week ago

I'm getting this error (slightly different place) in bingads 13.0.22.1. Did the fix from 13.0.20.1 not generalize?

java.util.ConcurrentModificationException:
  at java.util.ArrayList.checkForComodification (ArrayList.java:1095)
  at java.util.ArrayList.next (ArrayList.java:1049)
  at org.apache.cxf.jaxrs.provider.ProviderFactory.createMessageBodyWriter (ProviderFactory.java:570)
  at org.apache.cxf.jaxrs.provider.ProviderFactory.createMessageBodyWriterInterceptor (ProviderFactory.java:465 :cxf-rt-frontend-jaxrs-4.0.2.jar)
  at org.apache.cxf.jaxrs.client.AbstractClient.writeBody (AbstractClient.java:519)
  at  org.apache.cxf.jaxrs.client.WebClient$BodyWriter.doWriteBody (WebClient.java:1222)
  at org.apache.cxf.jaxrs.client.AbstractClient.handleMessage (AbstractClient.java:1223)
  at org.apache.cxf.phase.PhaseInterceptorChain.doIntercept (PhaseInterceptorChain.java:307
  at org.apache.cxf.jaxrs.client.AbstractClient.doRunInterceptorChain (AbstractClient.java:710)
  at org.apache.cxf.jaxrs.client.WebClient.prepareAsyncClient (WebClient.java:1000)
  at org.apache.cxf.jaxrs.client.WebClient.doInvokeAsync (WebClient.java:963)
  at org.apache.cxf.jaxrs.client.WebClient.doInvokeAsyncCallback (WebClient.java:952)
  at org.apache.cxf.jaxrs.client.AsyncInvokerImpl.file (AsyncInvokerImpl.java:211)
  at org.apache.cxf.jaxrs.client.AsyncInvokerImpl.post (AsyncInvokerImpl.java:94)
  at com.microsoft.bingads.internal.restful.RestfulServiceClient.processRequestAsync (RestfulServiceClient.java:360: microsoft.bingads-13.0.22.1.jar)
schabe77 commented 1 week ago

I would recommend using the jersey client. cxf is crap and not thread safe. synchronizing every call to cxf would make the bingads api not multi-threading capable anymore. I completely switched to jersey and have no concurrency problems anymore. unfortunately, to completely remove cxf, I needed a local hack that has to persist until #203 is fixed.

dmitchell commented 1 week ago

Thank you Christian. Yeah, I don't want to synchronize calls bc I'm intentionally multithreading them since I have to do 100s within a short period of time.

On Wed, Nov 13, 2024 at 2:30 AM Christian Habermehl < @.***> wrote:

I would recommend using the jersey client. cxf is crap and not thread safe. synchronizing every call to cxf would make the bingads api not multi-threading capable anymore. I completely switched to jersey and have no concurrency problems anymore. unfortunately, to completely remove cxf, I needed a local hack that has to persist until #203 https://github.com/BingAds/BingAds-Java-SDK/issues/203 is fixed.

— Reply to this email directly, view it on GitHub https://github.com/BingAds/BingAds-Java-SDK/issues/184#issuecomment-2472688929, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADUGD6JCEIXE6KQWOKIKMD2AL5ZBAVCNFSM6AAAAABHDUUJTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINZSGY4DQOJSHE . You are receiving this because you commented.Message ID: @.***>