search

mohamicorp / stash-jenkins-postreceive-webhook

Webhook used to notify Jenkins when commits are made to Stash
Other
138 stars 98 forks source link

RepositoryChangeListener should not block the event thread(s) while sending notifications to Jenkins #70

Open mheemskerk opened 10 years ago

mheemskerk commented 10 years ago

RepositoryChangeListener sends a notification to Jenkins on the event thread. This is a remote call that can block the event thread if the network is slow or the Jenkins server is slow to respond. If the Jenkins instance is structurally slow, all event threads will eventually get blocked trying to send notifications to Jenkins and event processing will fail.

Ideally, the notification to Jenkins should use an asynchronous HTTP request and not block the threads. See https://hc.apache.org/httpcomponents-asyncclient-dev/httpasyncclient/examples/org/apache/http/examples/nio/client/AsyncClientHttpExchangeFutureCallback.java for an example.

Michael Heemskerk Atlassian Stash

mikesir87 commented 10 years ago

Hello fellow Michael!

Thanks for the heads up. Looking at the RepositoryChangeListener, as well as all the other event listeners, if an event notification is to be sent, the event notifies the Notifier using the notifyBackground method, which uses an ExecutorService to do the actual work. So, the notification itself shouldn't be sent on the event thread. However, if I'm mistaken and misunderstood how Atlassian is handling the thread pools, just let me know and I'll be happy to use an AsyncClient.

mheemskerk commented 10 years ago

Hi Michael,

Hmmm... that's strange. Looking at the source code, RepositoryChangeListener does the right thing, but the thread dump (from what's reporting itself as 2.4.2) that we've got says otherwise:

"AtlassianEvent::pool-2-thread-1" prio=10 tid=0x00007ff1f0773800 nid=0x5988 runnable [0x00007ff1e8501000]
   java.lang.Thread.State: RUNNABLE
    at java.net.SocketInputStream.socketRead0(Native Method)
    at java.net.SocketInputStream.read(SocketInputStream.java:129)
    at org.apache.http.impl.io.AbstractSessionInputBuffer.fillBuffer(AbstractSessionInputBuffer.java:166)
    at org.apache.http.impl.io.SocketInputBuffer.fillBuffer(SocketInputBuffer.java:90)
    at org.apache.http.impl.io.AbstractSessionInputBuffer.readLine(AbstractSessionInputBuffer.java:281)
    at org.apache.http.impl.conn.DefaultHttpResponseParser.parseHead(DefaultHttpResponseParser.java:92)
    at org.apache.http.impl.conn.DefaultHttpResponseParser.parseHead(DefaultHttpResponseParser.java:62)
    at org.apache.http.impl.io.AbstractMessageParser.parse(AbstractMessageParser.java:254)
    at org.apache.http.impl.AbstractHttpClientConnection.receiveResponseHeader(AbstractHttpClientConnection.java:289)
    at org.apache.http.impl.conn.DefaultClientConnection.receiveResponseHeader(DefaultClientConnection.java:252)
    at org.apache.http.impl.conn.ManagedClientConnectionImpl.receiveResponseHeader(ManagedClientConnectionImpl.java:191)
    at org.apache.http.protocol.HttpRequestExecutor.doReceiveResponse(HttpRequestExecutor.java:300)
    at org.apache.http.protocol.HttpRequestExecutor.execute(HttpRequestExecutor.java:127)
    at org.apache.http.impl.client.DefaultRequestDirector.tryExecute(DefaultRequestDirector.java:712)
    at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:517)
    at org.apache.http.impl.client.AbstractHttpClient.execute(AbstractHttpClient.java:906)
    at org.apache.http.impl.client.AbstractHttpClient.execute(AbstractHttpClient.java:805)
    at org.apache.http.impl.client.AbstractHttpClient.execute(AbstractHttpClient.java:784)
    at com.nerdwin15.stash.webhook.Notifier.notify(Notifier.java:115)
    at com.nerdwin15.stash.webhook.Notifier.notify(Notifier.java:91)
    at com.nerdwin15.stash.webhook.RepositoryChangeListener.onRefsChangedEvent(RepositoryChangeListener.java:51)
    at sun.reflect.GeneratedMethodAccessor4658.invoke(Unknown Source)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at com.atlassian.event.internal.SingleParameterMethodListenerInvoker.invoke(SingleParameterMethodListenerInvoker.java:36)
    at com.atlassian.stash.internal.event.AsyncBatchingInvokersTransformer$AsyncInvokerBatch.invoke(AsyncBatchingInvokersTransformer.java:100)
    at com.atlassian.event.internal.AsynchronousAbleEventDispatcher$2.run(AsynchronousAbleEventDispatcher.java:66)
    at com.atlassian.sal.core.executor.ThreadLocalDelegateRunnable.run(ThreadLocalDelegateRunnable.java:38)
    at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
    at java.lang.Thread.run(Thread.java:662)

As you can see, Notifier.notify is called instead of Notifier.notifyBackground. I'm going to double check the exact version that's running.

mikesir87 commented 10 years ago

Any luck on your debugging?