[jira] [Commented] (JAMES-3477) MimeMessageCopyOnWriteProxy is not thread safe

2020-12-28 Thread ASF GitHub Bot (Jira)


[ 
https://issues.apache.org/jira/browse/JAMES-3477?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255823#comment-17255823
 ] 

ASF GitHub Bot commented on JAMES-3477:
---

chibenwa commented on pull request #280:
URL: https://github.com/apache/james-project/pull/280#issuecomment-751957047


   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:
us...@infra.apache.org


> MimeMessageCopyOnWriteProxy is not thread safe
> --
>
> Key: JAMES-3477
> URL: https://issues.apache.org/jira/browse/JAMES-3477
> Project: James Server
>  Issue Type: Wish
>Reporter: Benoit Tellier
>Priority: Major
>
> https://www.mail-archive.com/server-dev@james.apache.org/msg69221.html
> & 
> https://github.com/jeantil/james-project/commit/c0354ea21c5b0a8f6d46e9919f7db0d97db9eb23
> proves there is a concurrency issue in MimeMessageCopyOnWriteProxy class that 
> we need to investigate.
> It causes our test suite to be flacky.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



[jira] [Commented] (JAMES-3477) MimeMessageCopyOnWriteProxy is not thread safe

2020-12-28 Thread ASF GitHub Bot (Jira)


[ 
https://issues.apache.org/jira/browse/JAMES-3477?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255821#comment-17255821
 ] 

ASF GitHub Bot commented on JAMES-3477:
---

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:
us...@infra.apache.org


> MimeMessageCopyOnWriteProxy is not thread safe
> --
>
> Key: JAMES-3477
> URL: https://issues.apache.org/jira/browse/JAMES-3477
> Project: James Server
>  Issue Type: Wish
>Reporter: Benoit Tellier
>Priority: Major
>
> https://www.mail-archive.com/server-dev@james.apache.org/msg69221.html
> & 
> https://github.com/jeantil/james-project/commit/c0354ea21c5b0a8f6d46e9919f7db0d97db9eb23
> proves there is a concurrency issue in MimeMessageCopyOnWriteProxy class that 
> we need to investigate.
> It causes our test suite to be flacky.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



[jira] [Commented] (JAMES-3477) MimeMessageCopyOnWriteProxy is not thread safe

2020-12-28 Thread Benoit Tellier (Jira)


[ 
https://issues.apache.org/jira/browse/JAMES-3477?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255822#comment-17255822
 ] 

Benoit Tellier commented on JAMES-3477:
---

I started a mailig list discussion 
https://www.mail-archive.com/server-dev@james.apache.org/msg69361.html

> MimeMessageCopyOnWriteProxy is not thread safe
> --
>
> Key: JAMES-3477
> URL: https://issues.apache.org/jira/browse/JAMES-3477
> Project: James Server
>  Issue Type: Wish
>Reporter: Benoit Tellier
>Priority: Major
>
> https://www.mail-archive.com/server-dev@james.apache.org/msg69221.html
> & 
> https://github.com/jeantil/james-project/commit/c0354ea21c5b0a8f6d46e9919f7db0d97db9eb23
> proves there is a concurrency issue in MimeMessageCopyOnWriteProxy class that 
> we need to investigate.
> It causes our test suite to be flacky.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



The case of javax.mail MimeMessage CopyOnWrite optimization

2020-12-28 Thread Tellier Benoit
Hello there!

We had been discussing on GitHub recently about an optimization in james
core around the usage of MimeMessage.

Javax.mail MimeMessage is currently used to represent a message of an
email as part of the mail processing in James. It is part of the Mail
interface (mailet-api).

As Mail envelope is composed of several recipients, mail related
operations are performed once for all these recipients (we enqueue the
mail one time, we strip bcc one time etc...). Troubles arise when we
need different behaviors as part of mail processing across recipients
(think remote recipients, that needs there mail to be relayed, versus
local recipients that needs to be locally delivered). The email get's
duplicated (in MatcherSplitter) and the processing will then be distinct
for both entities. The underlying MimeMessage may - or may not be modified.

In order to prevent MimeMessage duplication in the event the underlying
MimeMessage is not modified, a Copy On Write mechanism was introduced (I
guess... Sorry, I was not there yet).

Upon his CI effort, Jean Helou with the help of Matthieu Baechler made
he unpleasant finding that this was not thread safe, that was leading to
build instabilities. The mailet processing happens in Camel, which is
multi-threaded. Concurrency issues arised between modifications, and
message disposal, when a same MimeMessage instance was shared. [1]

A first effort was to try to achieve thread-safety, which leaded to a
brittle double reetrant read-write locks in order to govern data access.
However, another performance enhancement bypassed these lock mechanism
(MimeMessageWrapper allows accessing the data as an InputStream instead
of requiring to copy it). The effort seemed overwhelming, not to metion
possible risks of dead-locks. [2]

We then came up with an always copy implementation [3]. Simpler,
safer... The underlying logic is to avoid trying being smarter than
mutability, and leverage immutability to achieve thread safety, which is
a classic functional programming idiom.

JMH benchmarks were conducted. We highlighter little performance
difference for small messages, in the percent realm for both memory
allocation and compute time. Differences are however higher for bigger
messages (~10%) for both metrics.

Please note that above 100KB the MimeMessage would be stored on disk,
thus limiting memory impact (see MimeMessageInputStreamSource). Maybe we
should make the threshold configurable, via a system property for instance?

I just want to further mentioned I encountered that very issue on a
production instance: the underlying email had been corrupted by the
above mentioned COW bug and kept throwing NullPointerExceptions every
time the content was accessed. This resulted (on top of
distributed-james) in a RabbitMQ nack of the message, that ended up in a
dead-letter queue. Replaying its processing required admin intervention
and had been interpreted by the user as an email loss...

To conclude this effort we (Jean an I) would like to merge the "Always
copy" pull request.

Also, would it be beneficial to write an ADR about this topic?

Thoughts?

Cheers,

Benoit

[1] The unfamous COW bug:
https://github.com/apache/james-project/pull/280/commits/09b5554bbcbbb98757910d59bac54f97ee1f8b4f

[2] The double nested reetrant read-write lock fix attempt:
https://github.com/apache/james-project/pull/280
[3] The always copy fix: https://github.com/apache/james-project/pull/282
[4] Benchmarks:
https://github.com/apache/james-project/pull/280#issuecomment-745211736
& https://github.com/apache/james-project/pull/280#issuecomment-745701937
[5] The JIRA ticket: https://issues.apache.org/jira/browse/JAMES-3477


-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



Re: Jenkins CI setup

2020-12-28 Thread Tellier Benoit
Hello Jean,

Nice work!

Le 28/12/2020 à 23:21, Jean Helou a écrit :
> Hello again jamers!
> 
> It's time for a new irregular report on the CI effort on apache infra 🎅 !
> 
> Let's start with the good news : today I finally reached a successful build
> https://builds.apache.org/blue/organizations/jenkins/james%2FApacheJames/detail/PR-268/45/pipeline
> (the first fully successful build on apache infra)

\o/

> 
> You can see in the pipeline that as discussed before the testing phase is
> split in 2 parts: stable tests vs unstable tests, failure in the first
> phase will fail the build, failures in the unstable phase will not be
> considered a build failure (but should still collect the failed tests in
> the reports, however the recent failures where mostly memory related in
> which case the surefire report is not generated :( )
> 
> Over the last 2 weeks and 45 build attempts, I tagged all failing tests as
> "Unstable", I also increased the heap in the forked surefire to resolve
> some of the OutOfMemoryException failures
> 
> At this stage I would really like to see this merged (if only to be able to
> evaluate dangerous changes such as
> https://github.com/apache/james-project/pull/282)

+1 I think this however deserves a separate thread. I will start it now.

> 
> You can look at https://github.com/apache/james-project/pull/268 to see
> which tests have been marked as Unstable. It was rebased on master this
> morning and I intend to clean up the history tonight.

Will do, maybe tomorrow. On the principles, it is a yes from my side.

As someone operating another CI, I want to play even unstable test on
every runs. Is there some adaptation needed to do this?

> I also removed some invasive logging from the webadmin test code (it used
> to log every single http request made in the tests) the full log is still a
> bit over 30MB...

Nice enhancement, it is likely some long-forgotten debug statements.
> 
> Best regards,
> Jean
> 
> On Fri, Dec 11, 2020 at 12:25 PM Jean Helou  wrote:
> 
>> I conclude that my effort to get CI working is cursed by the gods,
>> remember :
>>
 {"message":"No such image: quay.io/testcontainers/ryuk:0.2.3"}
>>>
>>> which repeats for most tests failures, this seems to be common enough
>>> that there is stack overflow for it
>>>
>>> https://stackoverflow.com/questions/61887363/testcontainers-cant-pull-ryuk-image-quay-io-is-not-reachable
>>> I have attempted to upgrade test containers to 1.15.0 (as it will pull
>>> ryuk from docker hub instead of quay.io since 1.14.3 and we were using
>>> 1.12)
>>> hopefully this will help :)
>>>
>>
>> A docker API change broke most of testcontainers versions, which won't be
>> able to pull the images if they are not already available locally !
>> https://github.com/testcontainers/testcontainers-java/issues/3574
>>> yes, this Docker API change applies to most of Testcontainers versions.
>>
>> They should release  a 1.15.1 to resolve the issue shortly, I have tried
>> explicitly pulling the image in the steps of running the tests but sadly it
>> doesn't seem to have helped :(
>>
>> jean
>>
>>>
> 

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



Re: JMAP vacation mailet

2020-12-28 Thread Tellier Benoit
Hello Jean,

Sorry for the late answer.

Le 27/12/2020 à 01:29, Jean Helou a écrit :
> Hi,
> 
> While playing around with assemblies, I got an error that vacation mailet
> configuration was missing when I added the JMAP module
> https://github.com/apache/james-project/blob/master/server/container/guice/protocols/jmap/src/main/java/org/apache/james/jmap/draft/JMAPModule.java#L122

This check was added to ensure the mailetcontainer was correctly
configured to answer people upon vacations.

The JMAP module will require this check, as far as I remember even if
the "JMAP protocol" itself is disabled (likely these checks should not
be performed if the configuration disables the JMAP protocol?)

On the subject of those checks, I personally do not like them yet...
It's a constraint but they are easy to cheat, the security they provide
is limited, but it restrict what as an administrator I could do... Like
re-organizing mailetcontainer.xml to perform a single call to
RecipientIsLocal (and thus to the LDAP) which has a performance and
resiliency impact.

> 
> I'm not very familiar with jmap and checked a draft at
> https://tools.ietf.org/html/draft-ietf-jmap-mail-16#section-1.3
> As far as I understand the vacation mailet it is an additional capability
> and could thus be made optional, am I correct ?

You are right.

Now, does it makes sense to expose the JMAP vacation extension in a
separate Guice modules? Is there some cases we want JMAP but not the
vacations?

By the way allowing people to write their own custom JMAP extensions
might become soon a topic, hopefully ;-)

> 
> thanks
> jean
> 

Cheers,

Benoit

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



[jira] [Commented] (JAMES-3431) Relay DSN options on RemoteDelivery

2020-12-28 Thread Benoit Tellier (Jira)


[ 
https://issues.apache.org/jira/browse/JAMES-3431?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255809#comment-17255809
 ] 

Benoit Tellier commented on JAMES-3431:
---

https://github.com/linagora/james-project/pull/4181 carries over DSN options to 
remote SMTP servers relying on javax.mail SMTP server.

Integration tests had been written using the Mock SMTP server previously 
modified.

Please note that the current work suffer from the following flows, that are 
limitations induces by the use of javax.mail:

 - No ORCPT support. This RCPT DSN parameter will be omitted on exchanges with 
remote SMTP servers. The impact is likely low: the value matches the RCPT TO, 
even in the case of mailing list rewrites. Maybe it is of some uses in the 
advent of aliases, for the sender MUA to display the actual user this mail was 
sent to.
 - As NOTIFY parameter is set on a per-SMTPMessage basis, we have to transmit 
several time the message (one time for each combinasons of NOTIFY parameters 
used for recipients of the message - recipients with the same notify parameters 
can be grouped together). The impact is low: it incurs a small performance 
cost. Also, I would be surprised MUAs set per-recipients values on sent emails 
and not global values.

We now remain to write the integration tests listed above.

> Relay DSN options on RemoteDelivery
> ---
>
> Key: JAMES-3431
> URL: https://issues.apache.org/jira/browse/JAMES-3431
> Project: James Server
>  Issue Type: Bug
>  Components: Remote Delivery, SMTPServer
>Affects Versions: 3.5.0
>Reporter: Karsten Otto
>Priority: Major
>
> Since James claims to support the DSN SMTP extension, it may receive a mail 
> submission according to [RFC 3461|https://tools.ietf.org/html/rfc3461]:
> {code:java}
> MAIL FROM: RET=HDRS ENVID=QQ314159
> RCPT TO: NOTIFY=SUCCESS,FAILURE,DELAY 
> ORCPT=rfc822;d...@ivory.edu
> RCPT TO: NOTIFY=NEVER{code}
> In this case James should
>  * remember the given DSN options (NOTIFY, ORCPT, RET, ENVID) for each 
> recipient, and
>  * provide the same options when relaying the mail to remote servers via the 
> RemoteDelivery mailet.
> (The DSN options should be accessible to other interested mailets as well, 
> e.g. for bounce processing.)
> Possibly related issues:
>  https://issues.apache.org/jira/browse/JAMES-322
>  https://issues.apache.org/jira/browse/JAMES-362
>  



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



[jira] [Created] (JAMES-3486) Adapt MailboxChangesMethodContract for stability against distributed environment

2020-12-28 Thread Lan Khuat (Jira)
Lan Khuat created JAMES-3486:


 Summary: Adapt MailboxChangesMethodContract for stability against 
distributed environment
 Key: JAMES-3486
 URL: https://issues.apache.org/jira/browse/JAMES-3486
 Project: James Server
  Issue Type: Sub-task
Reporter: Lan Khuat


h3. Objective

Because changes in distributed environment do not happen instantaneously, we 
need to adapt the contract so that the tests behave in a more reliable way.
h3. How

Before, we were storing a state manually as a reference point, then the 
change(s) that we interested in would be conducted after that. This will not 
work in distributed environment, since the reference state might be stored even 
before the provisioning process complete and leads to unpredictable result.
 * We will wait for a new state to be recorded successfully each time there is 
a change happen
 * Fetch them sequentially until all the preparation steps are completed
 * Mark the latest stage
 * Conduct the change that we are interested in
 * fetch the result with the latest state as reference point.

h3. DoD

Integration tests for MailboxChangesMethod should run reliably in distributed 
environment.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



[jira] [Commented] (JAMES-3477) MimeMessageCopyOnWriteProxy is not thread safe

2020-12-28 Thread ASF GitHub Bot (Jira)


[ 
https://issues.apache.org/jira/browse/JAMES-3477?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255778#comment-17255778
 ] 

ASF GitHub Bot commented on JAMES-3477:
---

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:
us...@infra.apache.org


> MimeMessageCopyOnWriteProxy is not thread safe
> --
>
> Key: JAMES-3477
> URL: https://issues.apache.org/jira/browse/JAMES-3477
> Project: James Server
>  Issue Type: Wish
>Reporter: Benoit Tellier
>Priority: Major
>
> https://www.mail-archive.com/server-dev@james.apache.org/msg69221.html
> & 
> https://github.com/jeantil/james-project/commit/c0354ea21c5b0a8f6d46e9919f7db0d97db9eb23
> proves there is a concurrency issue in MimeMessageCopyOnWriteProxy class that 
> we need to investigate.
> It causes our test suite to be flacky.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



[jira] [Commented] (JAMES-3446) Outlook Mobile App Imap request error

2020-12-28 Thread seungmin (Jira)


[ 
https://issues.apache.org/jira/browse/JAMES-3446?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255755#comment-17255755
 ] 

seungmin commented on JAMES-3446:
-

As a result, is there no way to solve the Outlook mobile problem?

> Outlook Mobile App Imap request error 
> --
>
> Key: JAMES-3446
> URL: https://issues.apache.org/jira/browse/JAMES-3446
> Project: James Server
>  Issue Type: Bug
>Affects Versions: 3.0-beta4
>Reporter: seungmin
>Priority: Major
> Fix For: 3.0.0
>
> Attachments: 20201117.cap, 20201117.cap, Capture d’écran de 
> 2020-11-17 09-57-45.png, image-2020-11-14-16-17-51-011.png, 
> image-2020-11-16-10-31-15-497.png, image-2020-11-16-14-23-24-498.png, 
> image-2020-11-16-14-25-06-005.png, image-2020-11-16-16-30-07-096.png, 
> image-2020-11-16-16-33-36-050.png, image-2020-11-16-16-33-37-781.png, 
> image-2020-11-16-16-58-33-360.png, image-2020-11-16-17-22-57-210.png, 
> image-2020-11-17-00-26-57-359.png, image-2020-11-17-00-27-10-493.png, 
> image-2020-11-17-00-27-58-590.png, image-2020-11-17-00-29-16-919.png, 
> image-2020-11-17-00-30-23-988.png, image-2020-11-17-00-31-42-025.png, 
> image-2020-11-18-16-09-15-340.png, image-2020-11-18-16-57-57-404.png, 
> image-2020-11-18-16-58-07-743.png, image-2020-11-18-18-08-55-845.png, 
> image-2020-11-19-16-51-07-279.png, image-2020-11-20-22-17-45-426.png, 
> image-2020-11-20-23-32-56-694.png, image-2020-11-21-02-45-12-876.png, 
> image-2020-11-30-00-48-06-447.png, image-2020-11-30-00-49-14-281.png, 
> image-2020-11-30-00-49-17-944.png, image-2020-11-30-00-55-59-761.png, 
> image-2020-11-30-00-56-26-139.png, image-2020-12-04-00-25-29-953.png
>
>
> !image-2020-11-14-16-17-51-011.png!
>  
> -  only outlook mobile app 
> Error while processing imap request
> cosumeWord source same master james source
> but gmail app, samsung mail app no problem
>  
> "only" outlook mobile issue



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



Re: Jenkins CI setup

2020-12-28 Thread Jean Helou
Hello again jamers!

It's time for a new irregular report on the CI effort on apache infra 🎅 !

Let's start with the good news : today I finally reached a successful build
https://builds.apache.org/blue/organizations/jenkins/james%2FApacheJames/detail/PR-268/45/pipeline
(the first fully successful build on apache infra)

You can see in the pipeline that as discussed before the testing phase is
split in 2 parts: stable tests vs unstable tests, failure in the first
phase will fail the build, failures in the unstable phase will not be
considered a build failure (but should still collect the failed tests in
the reports, however the recent failures where mostly memory related in
which case the surefire report is not generated :( )

Over the last 2 weeks and 45 build attempts, I tagged all failing tests as
"Unstable", I also increased the heap in the forked surefire to resolve
some of the OutOfMemoryException failures

At this stage I would really like to see this merged (if only to be able to
evaluate dangerous changes such as
https://github.com/apache/james-project/pull/282), I will make a

You can look at https://github.com/apache/james-project/pull/268 to see
which tests have been marked as Unstable. It was rebased on master this
morning and I intend to clean up the history tonight.
I also removed some invasive logging from the webadmin test code (it used
to log every single http request made in the tests) the full log is still a
bit over 30MB...

Best regards,
Jean

On Fri, Dec 11, 2020 at 12:25 PM Jean Helou  wrote:

> I conclude that my effort to get CI working is cursed by the gods,
> remember :
>
>> > {"message":"No such image: quay.io/testcontainers/ryuk:0.2.3"}
>>
>> which repeats for most tests failures, this seems to be common enough
>> that there is stack overflow for it
>>
>> https://stackoverflow.com/questions/61887363/testcontainers-cant-pull-ryuk-image-quay-io-is-not-reachable
>> I have attempted to upgrade test containers to 1.15.0 (as it will pull
>> ryuk from docker hub instead of quay.io since 1.14.3 and we were using
>> 1.12)
>> hopefully this will help :)
>>
>
> A docker API change broke most of testcontainers versions, which won't be
> able to pull the images if they are not already available locally !
> https://github.com/testcontainers/testcontainers-java/issues/3574
> > yes, this Docker API change applies to most of Testcontainers versions.
>
> They should release  a 1.15.1 to resolve the issue shortly, I have tried
> explicitly pulling the image in the steps of running the tests but sadly it
> doesn't seem to have helped :(
>
> jean
>
>>


[jira] [Commented] (JAMES-3477) MimeMessageCopyOnWriteProxy is not thread safe

2020-12-28 Thread ASF GitHub Bot (Jira)


[ 
https://issues.apache.org/jira/browse/JAMES-3477?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=1727#comment-1727
 ] 

ASF GitHub Bot commented on JAMES-3477:
---

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:
us...@infra.apache.org


> MimeMessageCopyOnWriteProxy is not thread safe
> --
>
> Key: JAMES-3477
> URL: https://issues.apache.org/jira/browse/JAMES-3477
> Project: James Server
>  Issue Type: Wish
>Reporter: Benoit Tellier
>Priority: Major
>
> https://www.mail-archive.com/server-dev@james.apache.org/msg69221.html
> & 
> https://github.com/jeantil/james-project/commit/c0354ea21c5b0a8f6d46e9919f7db0d97db9eb23
> proves there is a concurrency issue in MimeMessageCopyOnWriteProxy class that 
> we need to investigate.
> It causes our test suite to be flacky.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



[jira] [Commented] (JAMES-3477) MimeMessageCopyOnWriteProxy is not thread safe

2020-12-28 Thread ASF GitHub Bot (Jira)


[ 
https://issues.apache.org/jira/browse/JAMES-3477?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255517#comment-17255517
 ] 

ASF GitHub Bot commented on JAMES-3477:
---

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 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:
us...@infra.apache.org


> MimeMessageCopyOnWriteProxy is not thread safe
> --
>
> Key: JAMES-3477
> URL: https://issues.apache.org/jira/browse/JAMES-3477
> Project: James Server
>  Issue Type: Wish
>Reporter: Benoit Tellier
>Priority: Major
>
> https://www.mail-archive.com/server-dev@james.apache.org/msg69221.html
> & 
> https://github.com/jeantil/james-project/commit/c0354ea21c5b0a8f6d46e9919f7db0d97db9eb23
> proves there is a concurrency issue in MimeMessageCopyOnWriteProxy class that 
> we need to investigate.
> It causes our test suite to be flacky.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org



[jira] [Commented] (JAMES-3477) MimeMessageCopyOnWriteProxy is not thread safe

2020-12-28 Thread ASF GitHub Bot (Jira)


[ 
https://issues.apache.org/jira/browse/JAMES-3477?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255467#comment-17255467
 ] 

ASF GitHub Bot commented on JAMES-3477:
---

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 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 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:
us...@infra.apache.org


> MimeMessageCopyOnWriteProxy is not thread safe
> --
>
> Key: JAMES-3477
> URL: https://issues.apache.org/jira/browse/JAMES-3477
> Project: James Server
>  Issue Type: Wish
>Reporter: Benoit Tellier
>Priority: Major
>
> https://www.mail-archive.com/server-dev@james.apache.org/msg69221.html
> & 
> https://github.com/jeantil/james-project/commit/c0354ea21c5b0a8f6d46e9919f7db0d97db9eb23
> proves there is a concurrency issue in MimeMessageCopyOnWriteProxy class that 
> we need to investigate.
> It causes our test suite to be flacky.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

-
To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org
For additional commands, e-mail: server-dev-h...@james.apache.org