Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multipart performs blocking call in every instantiation #699

Open
jonathannaguin opened this issue Nov 3, 2023 · 9 comments · May be fixed by #716
Open

Multipart performs blocking call in every instantiation #699

jonathannaguin opened this issue Nov 3, 2023 · 9 comments · May be fixed by #716
Assignees
Labels

Comments

@jonathannaguin
Copy link

Describe the bug
When a Multipart object is created, the field streamProvider is also instantiated which relies ultimately on FactoryFinder#find (https://github.com/jakartaee/mail-api/blob/master/api/src/main/java/jakarta/mail/util/FactoryFinder.java#L35) that performs blocking operations.

To Reproduce
Create a new MimeMultipart object, for example

new MimeMultipart("mixed");

on a project with https://github.com/reactor/BlockHound installed.

Expected behavior
Either the FactoryFinder needs to be rewritten to not use non-blocking operations or Multipart could use a static reference so only it's blocked at boot up.

@jmehrens jmehrens added the enhancement New feature or request label Jan 22, 2024
@jmehrens
Copy link
Contributor

Related: #101

@jbescos
Copy link
Member

jbescos commented Feb 28, 2024

Hi @jonathannaguin, where is the blocking operation thing in FactoryFinder?, I cannot find it.

@jmehrens
Copy link
Contributor

@jbescos I assumed that the blocking was from Class::forName, ServiceLoader::load, etc.

My initial understanding of the fix is to follow advice from ServiceProvider "... Users are recommended to cache the result of this method." I don't think we are doing this everywhere but, I'm not sure if it is intended or not.

@jmehrens
Copy link
Contributor

The super class Multipart has a protected instance field that most likely causing this issue.

@jonathannaguin
Copy link
Author

That's right @jmehrens , https://github.com/jakartaee/mail-api/blob/master/api/src/main/java/jakarta/mail/Multipart.java#L69 will instantiate a StreamProvider (https://github.com/jakartaee/mail-api/blob/master/api/src/main/java/jakarta/mail/util/StreamProvider.java#L195) which will use FactoryFinder to find and instantiate given provider.

I was able to workaround some of this by setting some system properties so the Factory finds the classNames from the properties (avoiding factoryFromServiceLoader).

@jmehrens
Copy link
Contributor

jmehrens commented Feb 28, 2024

I think that field should be deprecated and removed when move to next major release. If we added a protected method instead of a field we could do searching of the provider of parent part which should have a session and or other child parts.

That said, the easy solution is to create a private static final field and use that to assign the protected instance field. I just have not done the research to figure out if we lose anything by not running this code for every instance.

@jbescos
Copy link
Member

jbescos commented Feb 29, 2024

The reason we depend on the StreamProvider is because we need an implementation of LineOutputStream and LineInputStream. Ironically we allow the implementations to provide its own, so they can build a better and more efficient code, meanwhile we make it less efficient adding this overhead of the StreamProvider.

I remember @lukasj and me were discussing this when we were splitting mail-api and angus-mail. There was a dependency in the API with the implementation, and we had to solve that introducing the StreamProvider. Initially it was added as a static field for performance reasons, but this makes problems with web containers like Weblogic or Tomcat, because once it is set, all the different web apps running there will have to use the same implementation.

Is it really worth this overhead of trying to find a StreamProvider somewhere to obtain those instances of only two interfaces?. In my opinion, it is better idea to have those implementations of LineOutputStream/LineInputStream within the Mail-API.

In case someone is able to create a more efficient LineOutputStream/LineInputStream implementation, we can just include it here.

CC @lukasj

@jmehrens
Copy link
Contributor

jmehrens commented Mar 1, 2024

That makes sense. I lean towards always getting the streamprovider from the session as that is a good spot to cache the lookup. In this case I think we can:

  1. Walk up the parents of the part to find a session to get provider. Meaning we have to delay lookup as long as possible so lookup is done in writeTo which in most cases should have an attached parent.
  2. If we can't find session, use default session with system properties and null authenticator.
  3. Last resort of cache and look up.

In any case, we would have to consider removing the instance field at some point to gain the benefit of this approach. I think this is a lower priority ticket.

@jbescos
Copy link
Member

jbescos commented Mar 1, 2024

The problem of using the default session is that its SteamProvider is static. I imagine 2 different wep-apps running in Tomcat, one with angus-mail and the other with other implementation of SteamProvider. This second app could use the angus-mail StreamProvider loaded by the first app, and I think that is undesired.

If it does not find the SteamProvider from its parents, I think it needs to create a new instance of it. Or we can just remove SteamProvider as I said before.

jbescos added a commit to jbescos/mail that referenced this issue Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants