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/26 05:00:35 UTC
[GitHub] [james-project] chibenwa opened a new pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
chibenwa opened a new pull request #282:
URL: https://github.com/apache/james-project/pull/282
Previous implementation was not thread safe as demonstrated
by @jeantil and @mbaechler. Upon concurrent writes and disposes
it leads to corruption of the underlying message, throwing
NullPointerExceptions and leads to data loss. [1]
Attempts to make it thread safe like [2] were complex, error
prone (double read-write lock involved!), and ultimately did
not manage to protect 'optimized' raw access to the underlying
data thus not fully addressing the concurrency issues.
Benchmarks were conducted in order to compare the Copy On
Write implementation with its always-copy equivalent. We
highlighted minor benefits of the copy-on-write version (
up to a few percent) both on compute time and memory allocation.
Only for bigger messages (10MB) did we reach higher differences,
of around 10%.
Note that the actual copy uses DeferredFileOutputStream
(commons.io) thus limits memory usage to 100KB upon copy.
[1] https://github.com/apache/james-project/pull/280/commits/09b5554bbcbbb98757910d59bac54f97ee1f8b4f
[2] https://github.com/apache/james-project/pull/280
[3] https://github.com/apache/james-project/pull/280#issuecomment-745211736
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751332936
> Hmm rethinking it all is this DeferredFileOutputStream safe with regards to
dispose ? What happens if we dispose the source bytes of a message that is
copied through DFOS?
`MimeMessageInputStreamSource` (holding the reference to the deferedFile) is disposed, each references to opened stream get cleaned up, and the file is deleted. We should be good...
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r548949128
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageInputStream.java
##########
@@ -40,19 +40,13 @@
* the message to wrap
* @param tryCast
* try to cast the {@link MimeMessage} to
- * {@link MimeMessageCopyOnWriteProxy} /
* {@link MimeMessageWrapper} to do some optimized processing if
* possible
* @throws MessagingException
*/
public MimeMessageInputStream(MimeMessage message, boolean tryCast) throws MessagingException {
MimeMessage m = message;
- // check if we need to use the wrapped message
- if (tryCast && m instanceof MimeMessageCopyOnWriteProxy) {
- m = ((MimeMessageCopyOnWriteProxy) m).getWrappedMessage();
- }
-
// check if we can use optimized operations
if (tryCast && m instanceof MimeMessageWrapper) {
Review comment:
Since the described failure occurred in 'optimized' access mode, is *this* optimization safe too ?
##########
File path: server/container/core/src/test/java/org/apache/james/server/core/MimeMessageTest.java
##########
@@ -311,9 +311,9 @@ public void testGetLineCount() throws Exception {
public void testMessageCloningViaCoW() throws Exception {
Review comment:
at a minimum the test name is misleading since there is no more COW :) you can probably drop this test
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageUtil.java
##########
@@ -75,10 +75,6 @@ public static void writeTo(MimeMessage message, OutputStream headerOs, OutputStr
*/
public static void writeTo(MimeMessage message, OutputStream headerOs, OutputStream bodyOs, String[] ignoreList) throws IOException, MessagingException {
MimeMessage testMessage = message;
Review comment:
this variable is no longer necessary, you can use message directlyon line 78 and 80 (because of the return) and as previously is the special casing for MimeMessageWrapper really useful ?
##########
File path: server/container/core/src/test/java/org/apache/james/server/core/MimeMessageTest.java
##########
@@ -336,9 +336,9 @@ public void testMessageCloningViaCoW() throws Exception {
public void testMessageCloningViaCoW2() throws Exception {
Review comment:
same as previous comment
##########
File path: server/container/core/src/test/java/org/apache/james/server/core/MimeMessageTest.java
##########
@@ -364,7 +364,7 @@ public void testMessageCloningViaCoW2() throws Exception {
public void testMessageCloningViaCoWSubjectLost() throws Exception {
Review comment:
same as previous comment
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-783856999
I merged this work
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751621215
> The initial pipeline ended up completing while the error handling pipeline was scheduling the mail for redelivery.
Both pipelines will work on different email copies now no?
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r549547128
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageInputStream.java
##########
@@ -40,19 +40,13 @@
* the message to wrap
* @param tryCast
* try to cast the {@link MimeMessage} to
- * {@link MimeMessageCopyOnWriteProxy} /
* {@link MimeMessageWrapper} to do some optimized processing if
* possible
* @throws MessagingException
*/
public MimeMessageInputStream(MimeMessage message, boolean tryCast) throws MessagingException {
MimeMessage m = message;
- // check if we need to use the wrapped message
- if (tryCast && m instanceof MimeMessageCopyOnWriteProxy) {
- m = ((MimeMessageCopyOnWriteProxy) m).getWrappedMessage();
- }
-
// check if we can use optimized operations
if (tryCast && m instanceof MimeMessageWrapper) {
Review comment:
Agree and already applied ;-)
----------------------------------------------------------------
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] mbaechler commented on pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
mbaechler commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-780076133
test this please
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r549617818
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageUtil.java
##########
@@ -75,10 +75,6 @@ public static void writeTo(MimeMessage message, OutputStream headerOs, OutputStr
*/
public static void writeTo(MimeMessage message, OutputStream headerOs, OutputStream bodyOs, String[] ignoreList) throws IOException, MessagingException {
MimeMessage testMessage = message;
Review comment:
ok, thanks for dropping the unnecessary variable
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-752832977
```
[ERROR] storingMessageWithSameKeyTwiceShouldUpdateMessageContent Time elapsed: 0.04 s <<< FAILURE!
org.opentest4j.MultipleFailuresError:
Multiple Failures (2 failures)
org.opentest4j.AssertionFailedError:
Expecting:
<"original body">
to be equal to:
<"modified content">
but was not.
at MailRepositoryContract.lambda$checkMailEquality$0(MailRepositoryContract.java:91)
org.opentest4j.AssertionFailedError:
Expecting:
<223L>
to be equal to:
<228L>
but was not.
at MailRepositoryContract.lambda$checkMailEquality$0(MailRepositoryContract.java:92)
[c4b6d407a19abec82e7c5178363d8abb452fbc72]
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] JDBCMailRepositoryTest.storingMessageWithSameKeyTwiceShouldUpdateMessageContent Multiple Failures (2 failures)
org.opentest4j.AssertionFailedError:
Expecting:
<"original body">
to be equal to:
<"modified content">
but was not.
at MailRepositoryContract.lambda$checkMailEquality$0(MailRepositoryContract.java:91)
org.opentest4j.AssertionFailedError:
Expecting:
<223L>
to be equal to:
<228L>
but was not.
at MailRepositoryContract.lambda$checkMailEquality$0(MailRepositoryContract.java:92)
[INFO]
[ERROR] Tests run: 39, Failures: 1, Errors: 0, Skipped: 0
```
That sounds like a real error.
Only JDBC Mail Repository is affected.
Now the question is "why"... Let's dig in crappy code...
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r548956275
##########
File path: server/container/core/src/test/java/org/apache/james/server/core/MimeMessageTest.java
##########
@@ -311,9 +311,9 @@ public void testGetLineCount() throws Exception {
public void testMessageCloningViaCoW() throws Exception {
Review comment:
I hesitated ;-)
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751667026
`If you are certain that concurrent access may no longer occur on MimeMessageInputStreamSource`
... How can I be sure of such a thing? :-)
` I suggest dropping the misleading synchronized.`
+1 this will make clear it is not thread safe... Just like we did in MimeMessageWrapper.
`on a side note: List<InputStream> streams the ordering property of lists doesn't seem to have any importance in the implementation, a Set would be more appropriate.`
true
`I guess we will have to wait and see if shared access to MimeMessageWrapper instances occurs or not.`
I pretty much agree with this statement...
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r548956195
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageInputStream.java
##########
@@ -40,19 +40,13 @@
* the message to wrap
* @param tryCast
* try to cast the {@link MimeMessage} to
- * {@link MimeMessageCopyOnWriteProxy} /
* {@link MimeMessageWrapper} to do some optimized processing if
* possible
* @throws MessagingException
*/
public MimeMessageInputStream(MimeMessage message, boolean tryCast) throws MessagingException {
MimeMessage m = message;
- // check if we need to use the wrapped message
- if (tryCast && m instanceof MimeMessageCopyOnWriteProxy) {
- m = ((MimeMessageCopyOnWriteProxy) m).getWrappedMessage();
- }
-
// check if we can use optimized operations
if (tryCast && m instanceof MimeMessageWrapper) {
Review comment:
That is a good question.
In a shared context likely not however if this MimeMessage is assumed not to be thread safe, and that each thread gets its own copy, we should be good, no?
Maybe we can remove the `synchronized` blocks in MimeMessageWrapper, and state in the Javadoc that it is not thread safe?
----------------------------------------------------------------
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 closed pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa closed pull request #282:
URL: https://github.com/apache/james-project/pull/282
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r550480120
##########
File path: server/mailet/mailetcontainer-camel/src/test/java/org/apache/james/mailetcontainer/impl/JamesMailetContextTest.java
##########
@@ -289,7 +291,7 @@ public void bounceShouldNotFailWhenNonConfiguredPostmaster() throws Exception {
.name("mail1")
.sender(mailAddress)
.addRecipient(mailAddress)
- .mimeMessage(MimeMessageUtil.defaultMimeMessage())
+ .mimeMessage(MimeMessageUtil.mimeMessageFromBytes("header: value\r\n".getBytes(UTF_8)))
Review comment:
considering how many times MimeMessageUtil.mimeMessageFromBytes("header: value\r\n".getBytes(UTF_8) is repeated over multiple tests, it could be worth a constant.
I think FakeMail is a good candidate to hold the constant or a factory method + constant if we want to avoid sharing the byte[]
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r548956250
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageUtil.java
##########
@@ -75,10 +75,6 @@ public static void writeTo(MimeMessage message, OutputStream headerOs, OutputStr
*/
public static void writeTo(MimeMessage message, OutputStream headerOs, OutputStream bodyOs, String[] ignoreList) throws IOException, MessagingException {
MimeMessage testMessage = message;
Review comment:
It do it as it allows to get an InputStream. javax.mail enforces a copy by writing to an output stream.
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751957067
I started a mailig list discussion https://www.mail-archive.com/server-dev@james.apache.org/msg69361.html
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil edited a comment on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751610341
>MimeMessageInputStreamSource (holding the reference to the deferedFile) is disposed, each references to opened stream get cleaned up, and the file is *deleted*. We should be good...
Thats what I meant : I may be reading MimeMessageInputStreamSource's code incorrectly but I don't see a reason that dispose cannot be called by a thread `t1` while another thread `t2` is trying to read the contents of one of the inputstream returned by `getInputStream()`
`t2` is going to get `java.io.IOException: inputstream is closed` because `t1` disposed the message and we are back to data loss territory.
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751610341
>MimeMessageInputStreamSource (holding the reference to the deferedFile) is disposed, each references to opened stream get cleaned up, and the file is *deleted*. We should be good...
Thats what I meant : I may be reading MimeMessageInputStreamSource's code incorrectly but I don't see a reason that dispose cannot be called by a thread `t1` while another thread `t2` is trying to read the contents of one of the inputstream returned by `getInputStream()`
`t2` is going to get `java.io.IOException: inputstream is closed` because `t1` disposed the message and we are back to data loss territory.
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r549327693
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageInputStream.java
##########
@@ -40,19 +40,13 @@
* the message to wrap
* @param tryCast
* try to cast the {@link MimeMessage} to
- * {@link MimeMessageCopyOnWriteProxy} /
* {@link MimeMessageWrapper} to do some optimized processing if
* possible
* @throws MessagingException
*/
public MimeMessageInputStream(MimeMessage message, boolean tryCast) throws MessagingException {
MimeMessage m = message;
- // check if we need to use the wrapped message
- if (tryCast && m instanceof MimeMessageCopyOnWriteProxy) {
- m = ((MimeMessageCopyOnWriteProxy) m).getWrappedMessage();
- }
-
// check if we can use optimized operations
if (tryCast && m instanceof MimeMessageWrapper) {
Review comment:
you definitely want to remove all the synchronized blocks on MimeMessageWrapper since it is definitely not thread safe (f only because it relies on MimeMessageInputStreamSource) which itself isn't thread safe. This will both remove the illusion of thread safety, provide performance improvements and possibly let the integration test fail faster if there are concurrent access left.
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751619159
`if memory serves vacation mailet threw an error on a mail which triggered the james error handling pipeline asynchronously.`
Sorry, can you elaborate? I do not understand this statement...
`The initial pipeline ended up completing while the error handling pipeline was scheduling the mail for redelivery. `
Okay I kind of grasp it... Because the response from VacationMailet is an actual reply of the original email.
`This triggered a NPE because of the MMCOWP implementation at the time, the currrent implementation may yield IOException instead of NPEs ...`
Even if we make sure `reply` returns a 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] chibenwa commented on pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-752833540
39 more maven modules to go!
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r550480597
##########
File path: server/protocols/webadmin/webadmin-mailrepository/src/test/java/org/apache/james/webadmin/routes/MailRepositoriesRoutesTest.java
##########
@@ -107,6 +109,7 @@
private static final MailQueueName CUSTOM_QUEUE = MailQueueName.of("customQueue");
private static final String NAME_1 = "name1";
private static final String NAME_2 = "name2";
+ private static final byte[] MESSAGE_BYTES = "header: value \r\n".getBytes(UTF_8);
Review comment:
another occurence where the value itself has been extracted to a local constant
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751631891
> Both pipelines will work on different email copies now no?
In the event that there are 2 different MimeMessageWrapper instances, are we certain that DeferredOutputStream will have finished copying the underlying file ?
I was worried that the file storing the email data for t1 might be deleted before the file storing the data for t2 is completely written ending up in a MimeMessageWrapper instance in t2 that's backed by a corrupted file.
I read through more code and it looks like DeferredOutputStream does make a defensive copy (I was misled by the `deferred` in the name) so 2 different instances would have 2 complete files as soon as instance creation returns.
That leaves the `synchronized` flag on `MimeMessageInputStreamSource#getInputStream` which suggests concurrent access may occur there with the possible outcome I mentionned. If you are certain that concurrent access may no longer occur on `MimeMessageInputStreamSource` I suggest dropping the misleading `synchronized`. I assume that the synchronization was added to avoid/fix a `ConcurrentModificationException` on the `List<InputStream> streams`. The `synchronized` is actually a lie since a `ConcurrentModificationException` could still be triggererd by threads calling `getInputStream` and dispose concurrently
on a side note: `List<InputStream> streams` the ordering property of lists doesn't seem to have any importance in the implementation, a `Set` would be more appropriate.
I guess we will have to wait and see if shared access to MimeMessageWrapper instances occurs or not.
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-755263670
Sorry I see this, but I won't fix these remarks before the 20th January.
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r550479649
##########
File path: server/data/data-jdbc/src/main/java/org/apache/james/mailrepository/jdbc/JDBCMailRepository.java
##########
@@ -551,22 +545,6 @@ private boolean checkMessageExists(Mail mc, Connection conn) throws SQLException
}
}
- private boolean saveBodyRequired(Mail mc) throws MessagingException {
- boolean saveBody;
- MimeMessage messageBody = mc.getMessage();
-
- if (messageBody instanceof MimeMessageWrapper) {
- MimeMessageWrapper message = (MimeMessageWrapper) messageBody;
Review comment:
one less special casing of MimeMessageWrapper ! @chibenwa for the win ! :+1:
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751614173
> Thats what I meant : I may be reading MimeMessageInputStreamSource's code incorrectly but I don't see a reason that dispose cannot be called by a thread t1 while another thread t2 is trying to read the contents of one of the inputstream returned by getInputStream()
t2 is going to get java.io.IOException: inputstream is closed because t1 disposed the message and we are back to data loss territory.
Ahhh mutability.
Do you have exemples of situations when this can happen? I think a Mail object and thus underlying MimeMessage is only disposed when processing is finished...
What do you propose then?
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-751616212
> Do you have exemples of situations when this can happen?
We discovered all this thread safety issue when debugging flaky tests: if memory serves vacation mailet threw an error on a mail which triggered the james error handling pipeline asynchronously. The initial pipeline ended up completing while the error handling pipeline was scheduling the mail for redelivery. This triggered a NPE because of the MMCOWP implementation at the time, the currrent implementation may yield IOException instead of NPEs ...
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-780434060
:green_apple: on the CI
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-779080078
I spent hours on it, to link the failures to SpamAssassin timing out
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-780238563
test this please
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r550481314
##########
File path: server/protocols/webadmin/webadmin-mailrepository/src/test/java/org/apache/james/webadmin/routes/MailRepositoriesRoutesTest.java
##########
@@ -107,6 +109,7 @@
private static final MailQueueName CUSTOM_QUEUE = MailQueueName.of("customQueue");
private static final String NAME_1 = "name1";
private static final String NAME_2 = "name2";
+ private static final byte[] MESSAGE_BYTES = "header: value \r\n".getBytes(UTF_8);
Review comment:
another occurence where the value itself has been extracted to a local constant
##########
File path: server/mailet/mailetcontainer-camel/src/test/java/org/apache/james/mailetcontainer/impl/JamesMailetContextTest.java
##########
@@ -289,7 +291,7 @@ public void bounceShouldNotFailWhenNonConfiguredPostmaster() throws Exception {
.name("mail1")
.sender(mailAddress)
.addRecipient(mailAddress)
- .mimeMessage(MimeMessageUtil.defaultMimeMessage())
+ .mimeMessage(MimeMessageUtil.mimeMessageFromBytes("header: value\r\n".getBytes(UTF_8)))
Review comment:
considering how many times MimeMessageUtil.mimeMessageFromBytes("header: value\r\n".getBytes(UTF_8) is repeated over multiple tests, it could be worth a constant.
I think FakeMail is a good candidate to hold the constant or a factory method + constant if we want to avoid sharing the byte[]
##########
File path: server/protocols/webadmin/webadmin-mailrepository/src/test/java/org/apache/james/webadmin/service/ReprocessingServiceTest.java
##########
@@ -61,6 +63,7 @@
private static final MailQueueName SPOOL = MailQueueName.of("spool");
private static final Consumer<MailKey> NOOP_CONSUMER = key -> { };
private static final Optional<String> NO_TARGET_PROCESSOR = Optional.empty();
+ private static final byte[] MESSAGE_BYTES = "header: value \r\n".getBytes(UTF_8);
Review comment:
more of the same
----------------------------------------------------------------
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 #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #282:
URL: https://github.com/apache/james-project/pull/282#issuecomment-778996072
Still getting...
```
05:01:40.417 [ERROR] o.a.j.m.i.c.CamelProcessor - Exception calling Sieve: Mail message to be spooled cannot be null.
java.lang.NullPointerException: Mail message to be spooled cannot be null.
at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:895)
at org.apache.james.transport.mailets.jsieve.delivery.SieveExecutor.execute(SieveExecutor.java:120)
at org.apache.james.transport.mailets.Sieve.service(Sieve.java:75)
at org.apache.james.mailetcontainer.impl.camel.CamelProcessor.process(CamelProcessor.java:77)
at org.apache.james.mailetcontainer.impl.camel.CamelMailetProcessor$MailetContainerRouteBuilder.handleMailet(CamelMailetProcessor.java:176)
at org.apache.james.mailetcontainer.impl.camel.CamelMailetProcessor$MailetContainerRouteBuilder.lambda$configure$0(CamelMailetProcessor.java:153)
at org.apache.camel.processor.DelegateSyncProcessor.process(DelegateSyncProcessor.java:63)
at org.apache.camel.processor.RedeliveryErrorHandler.process(RedeliveryErrorHandler.java:548)
at org.apache.camel.processor.CamelInternalProcessor.process(CamelInternalProcessor.java:201)
at org.apache.camel.processor.RedeliveryErrorHandler.process(RedeliveryErrorHandler.java:548)
```
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r550480120
##########
File path: server/mailet/mailetcontainer-camel/src/test/java/org/apache/james/mailetcontainer/impl/JamesMailetContextTest.java
##########
@@ -289,7 +291,7 @@ public void bounceShouldNotFailWhenNonConfiguredPostmaster() throws Exception {
.name("mail1")
.sender(mailAddress)
.addRecipient(mailAddress)
- .mimeMessage(MimeMessageUtil.defaultMimeMessage())
+ .mimeMessage(MimeMessageUtil.mimeMessageFromBytes("header: value\r\n".getBytes(UTF_8)))
Review comment:
considering how many times MimeMessageUtil.mimeMessageFromBytes("header: value\r\n".getBytes(UTF_8) is repeated over multiple tests, it could be worth a constant.
----------------------------------------------------------------
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 a change in pull request #282: JAMES-3477 Drop MimeMessageCopyOnWriteProxy
Posted by GitBox <gi...@apache.org>.
jeantil commented on a change in pull request #282:
URL: https://github.com/apache/james-project/pull/282#discussion_r550480597
##########
File path: server/protocols/webadmin/webadmin-mailrepository/src/test/java/org/apache/james/webadmin/routes/MailRepositoriesRoutesTest.java
##########
@@ -107,6 +109,7 @@
private static final MailQueueName CUSTOM_QUEUE = MailQueueName.of("customQueue");
private static final String NAME_1 = "name1";
private static final String NAME_2 = "name2";
+ private static final byte[] MESSAGE_BYTES = "header: value \r\n".getBytes(UTF_8);
Review comment:
another occurence where the value itself has been extracted to a local constant
a Good candidate would be a constant or a factory method on FakeMail if we want to avoid sharing the byte[]
----------------------------------------------------------------
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