You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2020/12/22 13:50:57 UTC
[GitHub] [james-project] jeantil opened a new pull request #281: JAMES-3477 Disable copy on write through system property
jeantil opened a new pull request #281:
URL: https://github.com/apache/james-project/pull/281
Builds upon @chibenwa's proposed implementation in #280 but makes it easy to fully disable copy on write behaviour and replace it with always copy.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] jeantil commented on pull request #281: JAMES-3477 Disable copy on write through system property
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #281:
URL: https://github.com/apache/james-project/pull/281#issuecomment-751323163
I have seen #282 and I'll definitely vote for it :)
Can you confirm the following about the logs above
- you were using the patch from #280 in a live system
- you still observed the concurrency issue with the patch applied
And expand on this from #282
> Attempts to make it thread safe like [2] [...] ultimately did not manage to protect 'optimized' raw access to the underlying data thus not fully addressing the concurrency issues.
Do you know what the 'optimized' raw access you speak of is and where and when it used to occur ? I'm curious because dispose would set the ref to underlying to null but
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] jeantil commented on pull request #281: JAMES-3477 Disable copy on write through system property
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #281:
URL: https://github.com/apache/james-project/pull/281#issuecomment-751288931
> What about implementing a strategy pattern instead of making MimeMessageCOW even more complex?
I don't see the point of introducing a strategy pattern here : we only have 2 implementations and once we know which is better, the other one will be dropped. do you have alternative implementation ideas where it would make sense to let multiple implementations live side by side ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] jeantil commented on pull request #281: JAMES-3477 Disable copy on write through system property
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #281:
URL: https://github.com/apache/james-project/pull/281#issuecomment-750293565
I don't know if we should document it:
This is really an internal implementation detail, should it really be configurable (adding configuration means the already complex code becomes even more complex)
I only introduced the flag to let *us* more easily benchmark both implementations.
I feel the loosing implementation should simply be removed once we have the benchmarks results as there is no point in leaving an inferior implementation in the code.
I'll update the comment to reflect that instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #281: JAMES-3477 Disable copy on write through system property
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #281:
URL: https://github.com/apache/james-project/pull/281#issuecomment-751309668
BTW this morning in the logs...
```
{
"_index": "fluentbit-james-2020.12.23",
"_type": "docker",
"_id": "TJFTj3YBdqSpI3q1b9tM",
"_score": 1,
"_source": {
"@timestamp": "2020-12-23T11:19:35.980Z",
"timestamp": "2020-12-23T11:19:35.980Z",
"level": "ERROR",
"thread": "elastic-4415",
"logger": "org.apache.james.mailetcontainer.impl.JamesMailSpooler",
"message": "Could not apply standard error handling for bc947010-4510-11eb-a696-31d91ae7ef70, defaulting to nack",
"context": "default",
"exception": "java.lang.NullPointerException: null\n\tat org.apache.james.blob.mail.MimeMessageStore$MimeMessageEncoder.messageToArray(MimeMessageStore.java:90)\n\tat org.apache.james.blob.mail.MimeMessageStore$MimeMessageEncoder.encode(MimeMessageStore.java:76)\n\tat org.apache.james.blob.mail.MimeMessageStore$MimeMessageEncoder.encode(MimeMessageStore.java:72)\n\tat org.apache.james.blob.api.Store$Impl.save(Store.java:84)\n\tat org.apache.james.queue.rabbitmq.Enqueuer.saveMail(Enqueuer.java:89)\n\tat org.apache.james.queue.rabbitmq.Enqueuer.enQueue(Enqueuer.java:72)\n\tat org.apache.james.queue.rabbitmq.RabbitMQMailQueue.lambda$enQueue$0(RabbitMQMailQueue.java:85)\n\tat com.github.fge.lambdas.runnable.RunnableChainer.lambda$sneakyThrow$215(RunnableChainer.java:71)\n\tat org.apache.james.metrics.api.MetricFactory.lambda$runPublishingTimerMetric$0(MetricFactory.java:58)\n\tat org.apache.james.metrics.api.MetricFactory.decorateSupplierWithTimerMetric(MetricFactory.java:37)\n\ta
t org.apache.james.metrics.api.MetricFactory.runPublishingTimerMetric(MetricFactory.java:57)\n\tat org.apache.james.queue.rabbitmq.RabbitMQMailQueue.enQueue(RabbitMQMailQueue.java:84)\n\tat org.apache.james.mailetcontainer.impl.JamesMailSpooler.reEnqueue(JamesMailSpooler.java:199)\n\tat org.apache.james.mailetcontainer.impl.JamesMailSpooler.handleError(JamesMailSpooler.java:181)\n\tat org.apache.james.mailetcontainer.impl.JamesMailSpooler.performProcessMail(JamesMailSpooler.java:166)\n\tat org.apache.james.mailetcontainer.impl.JamesMailSpooler.lambda$processMail$6(JamesMailSpooler.java:152)\n\tat reactor.core.publisher.MonoRunnable.subscribe(MonoRunnable.java:49)\n\tat reactor.core.publisher.MonoUsing.subscribe(MonoUsing.java:109)\n\tat reactor.core.publisher.Mono.subscribe(Mono.java:4210)\n\tat reactor.core.publisher.FluxFlatMap.trySubscribeScalarMap(FluxFlatMap.java:199)\n\tat reactor.core.publisher.MonoFlatMap.subscribeOrReturn(MonoFlatMap.java:53)\n\tat reactor.core.publisher.Mo
no.subscribe(Mono.java:4195)\n\tat reactor.core.publisher.MonoSubscribeOn$SubscribeOnSubscriber.run(MonoSubscribeOn.java:124)\n\tat reactor.core.scheduler.WorkerTask.call(WorkerTask.java:84)\n\tat reactor.core.scheduler.WorkerTask.call(WorkerTask.java:37)\n\tat java.base/java.util.concurrent.FutureTask.run(Unknown Source)\n\tat java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)\n\tat java.base/java.lang.Thread.run(Unknown Source)\n",
"log": "{\"timestamp\":\"2020-12-23T11:19:35.980Z\",\"level\":\"ERROR\",\"thread\":\"elastic-4415\",\"logger\":\"org.apache.james.mailetcontainer.impl.JamesMailSpooler\",\"message\":\"Could not apply standard error handling for bc947010-4510-11eb-a696-31d91ae7ef70, defaulting to nack\",\"context\":\"default\",\"exception\":\"java.lang.NullPointerException: null\\n\\tat org.apache.james.blob.mail.MimeMessageStore$MimeMessageEncoder.messageToArray(MimeMessageStore.java:90)\\n\\tat org.apache.james.blob.mail.MimeMessageStore$MimeMessageEncoder.encode(MimeMessageStore.java:76)\\n\\tat org.apache.james.blob.mail.MimeMessageStore$MimeMessageEncoder.encode(MimeMessageStore.java:72)\\n\\tat org.apache.james.blob.api.Store$Impl.save(Store.java:84)\\n\\tat org.apache.james.queue.rabbitmq.Enqueuer.saveMail(Enqueuer.java:89)\\n\\tat org.apache.james.queue.rabbitmq.Enqueuer.enQueue(Enqueuer.java:72)\\n\\tat org.apache.james.queue.rabbitmq.RabbitMQMailQueue.lambda$enQueue$0(RabbitMQMailQueu
e.java:85)\\n\\tat com.github.fge.lambdas.runnable.RunnableChainer.lambda$sneakyThrow$215(RunnableChainer.java:71)\\n\\tat org.apache.james.metrics.api.MetricFactory.lambda$runPublishingTimerMetric$0(MetricFactory.java:58)\\n\\tat org.apache.james.metrics.api.MetricFactory.decorateSupplierWithTimerMetric(MetricFactory.java:37)\\n\\tat org.apache.james.metrics.api.MetricFactory.runPublishingTimerMetric(MetricFactory.java:57)\\n\\tat org.apache.james.queue.rabbitmq.RabbitMQMailQueue.enQueue(RabbitMQMailQueue.java:84)\\n\\tat org.apache.james.mailetcontainer.impl.JamesMailSpooler.reEnqueue(JamesMailSpooler.java:199)\\n\\tat org.apache.james.mailetcontainer.impl.JamesMailSpooler.handleError(JamesMailSpooler.java:181)\\n\\tat org.apache.james.mailetcontainer.impl.JamesMailSpooler.performProcessMail(JamesMailSpooler.java:166)\\n\\tat org.apache.james.mailetcontainer.impl.JamesMailSpooler.lambda$processMail$6(JamesMailSpooler.java:152)\\n\\tat reactor.core.publisher.MonoRunnable.subscribe(
MonoRunnable.java:49)\\n\\tat reactor.core.publisher.MonoUsing.subscribe(MonoUsing.java:109)\\n\\tat reactor.core.publisher.Mono.subscribe(Mono.java:4210)\\n\\tat reactor.core.publisher.FluxFlatMap.trySubscribeScalarMap(FluxFlatMap.java:199)\\n\\tat reactor.core.publisher.MonoFlatMap.subscribeOrReturn(MonoFlatMap.java:53)\\n\\tat reactor.core.publisher.Mono.subscribe(Mono.java:4195)\\n\\tat reactor.core.publisher.MonoSubscribeOn$SubscribeOnSubscriber.run(MonoSubscribeOn.java:124)\\n\\tat reactor.core.scheduler.WorkerTask.call(WorkerTask.java:84)\\n\\tat reactor.core.scheduler.WorkerTask.call(WorkerTask.java:37)\\n\\tat java.base/java.util.concurrent.FutureTask.run(Unknown Source)\\n\\tat java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source)\\n\\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)\\n\\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)\\n\\tat java.base/java.lang.Thre
ad.run(Unknown Source)\\n\"}\n",
"stream": "stdout",
"time": "2020-12-23T11:19:35.981461976Z"
},
"fields": {
"@timestamp": [
"2020-12-23T11:19:35.980Z"
],
"time": [
"2020-12-23T11:19:35.981Z"
],
"timestamp": [
"2020-12-23T11:19:35.980Z"
]
}
}
```
Merry Xmas ;-)
NPE upon mail processing => sounds fishy to me. It's so bad that we could not even save that message into a mailrepository, nor send in back to the queue... As if the content had been corrupted! :-(
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] jeantil edited a comment on pull request #281: JAMES-3477 Disable copy on write through system property
Posted by GitBox <gi...@apache.org>.
jeantil edited a comment on pull request #281:
URL: https://github.com/apache/james-project/pull/281#issuecomment-751323163
I have seen #282 and I'll definitely vote for it :)
Can you confirm the following about the logs above
- you were using the patch from #280 in a live system
- you still observed the concurrency issue with the patch applied
And expand on this from #282
> Attempts to make it thread safe like [2] [...] ultimately did not manage to protect 'optimized' raw access to the underlying data thus not fully addressing the concurrency issues.
Do you know what the 'optimized' raw access you speak of is and where and when it used to occur ? I'm curious because dispose would set the ref to underlying to null but if the 'optimized' access already had the reference it shouldn't have failed...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #281: JAMES-3477 Disable copy on write through system property
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #281:
URL: https://github.com/apache/james-project/pull/281#issuecomment-751245266
> What about implementing a strategy pattern instead of making MimeMessageCOW even more complex?
As the intent is `easing testing` we can live with a bit of dirty code I guess, as @jeantil told us this code was not meant to be merged...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] jeantil closed pull request #281: JAMES-3477 Disable copy on write through system property
Posted by GitBox <gi...@apache.org>.
jeantil closed pull request #281:
URL: https://github.com/apache/james-project/pull/281
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #281: JAMES-3477 Disable copy on write through system property
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #281:
URL: https://github.com/apache/james-project/pull/281#issuecomment-751347054
> Do you know what the 'optimized' raw access you speak of is?
It was accessing the shared underlying MimeMessageWrapper to get an InputStream out of it (instead of writeTo + OutputSream)
> where and when it used to occur ?
It happens in most 'core' aware classes, anytime we want to have an InputStream version of the message.
> I'm curious because dispose would set the ref to underlying to null but if the 'optimized' access already had the reference it shouldn't have failed...
See https://github.com/apache/james-project/pull/280#commitcomment-45090717
The message held by the reference was exposed unprotected. Thus: when you have a COW message, leveraging the benefits of MimeMessageWrapper was unsafe.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org