terrischwartz / tus_servlet

java servlet implementing server side of tus file upload protocol
Other
24 stars 10 forks source link

Complete container coverage #1

Open vincent-vieira opened 8 years ago

vincent-vieira commented 8 years ago

From my own experience with this module, seems like it's not working with latest release of Undertow (I can report an issue in a separate ticket if needed) and it would be a great idea to test this. I have not tested using Tomcat, and the Jetty version used in the pom is like 6 years old, so a little update would be nice.

A good idea would be to make use of embedded distributions of certain containers to make proper tests in order to verify this library's wellbeing, instead of just testing that cURL command passes.

terrischwartz commented 8 years ago

Hi Vincent,

Thanks for trying out the library and providing feedback. I'm not familiar with Undertow. Would you mind providing more description about what isn't working right?

I last tested this a couple of months ago; if I have a chance latter today, I'll verify that it still works with jetty and/or tomcat. I manually test via a browser as well as running a few quick tests with curl. I'm afraid I don't have time to dedicate to creating a more thorough test suite, though I believe the tus.io group is working on a test framework for server implementations.

The mvn pom.xml should handle pulling down the needed jars, old though they may be, and those jars shouldn't interfere with any other versions you may need for other projects. Why would it be more desirable to include the jetty jar in a distribution? (Also, I'll update the pom.xml to use a more recent version of jetty. I hadn't realized the one I specified was so out of date).

It would be nice to know whether the problem you're experiencing exists when run with jetty and/or tomcat or whether it's specific to integration with Undertow.

vincent-vieira commented 8 years ago

Hey there,

If this implementation is designed to be the Java main one and if the tus.io group doesn't work on a Java server implementation, I can contribute and create a pull request with a more thorough test suite.

With Undertow, I had an exception with NIO handling and writing (java.nio.channels.ClosedChannelException). I haven't investigated more since I didn't have much time. But if we write tests, this exception will probably pop up and we'll know much more about it.

This is probably due to be declared on another issue but unfortunately yes, I think they interfere. Using latest version of Undertow forced me to include your project as a dependency while excluding Jetty since this old version only supports Servlet API 2.5 and Undertow works with Servlet API 3.1. I think that by design, your module should include the latest Servlet API as a provided dependency, and the guy using it should provide its matching implementation by the use of a container (that container being either embedded or unpacking a war). But, as I said earlier, this is a topic to be covered with a separate issue.

Anyway, your library helped me out but some visible drawbacks need to be improved, and I'm offering my help on all these topics.

Cheers, Vincent

terrischwartz commented 8 years ago

Hi Vincent,

Thanks for explaining. I see what you mean now about the packaging now.

I would love to have your help improving the package. I'm also very curious about the NIO exception. Does it always occur, even with tiny uploads?

How do you propose writing and running tests?

Terri


From: Vincent Vieira [notifications@github.com] Sent: Tuesday, August 16, 2016 9:18 AM To: terrischwartz/tus_servlet Cc: Schwartz, Terri; Comment Subject: Re: [terrischwartz/tus_servlet] Complete container coverage (#1)

Hey there,

If this implementation is designed to be the Java main one and if the tus.io group don't work on a Java server implementation, I can contribute and create a pull request with a more thorough test suite.

With Undertow, I had an exception with NIO handling and writing (java.nio.channels.ClosedChannelException). I haven't investigated more since I didn't have much time. But if we write tests, this exception will probably pop up and we'll know much more about it.

This is probably due to be declared on another issue but unfortunately yes, I think they interfere. Using latest version of Undertow forced me to include your project as a dependency while excluding Jetty since this old version only supports Servlet API 2.5 and Undertow works with Servlet API 3.1. I think that by design, your module should include the latest Servlet API as a provided dependency, and the guy using it should provide its matching implementation by the use of a container (that container being either embedded or unpacking a war). But, as I said earlier, this is a topic to be covered with a separate issue.

Anyway, your library helped me out but some visible drawbacks need to be improved, and I'm offering my help on all these topics.

Cheers, Vincent

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/terrischwartz/tus_servlet/issues/1#issuecomment-240154479, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AKrpNPf9_rRfJ707atrShz8Jf2cUsCGfks5qgeLNgaJpZM4Jlh1G.

vincent-vieira commented 8 years ago

Terri,

About the packaging, and if you're ok with it, I'll create a specific issue covering the main issue and the possible workarounds in order to do better.

About the NIO exception, I used the official Java client to upload a tiny file of 1.5kB, so yes.

About tests, I suggest that we keep your tests. Those bash scripts can be run in a CI environment (that would be also great, using TravisCI for instance) or through a custom Maven execution when running tests.

In order to cover in a more comprehensive way all containers, I suggest that we update the module's POM in order to include all latest embedded containers (with a test scope of course) and run a test for each one of them. I'm sure that Jetty, Tomcat and Undertow are provided out of the box. I'm 80% sure that JBoss and Glassfish can be run in tests using the EmbeddedEJBContainer, this container will be simply configured as a Servlet one.

I can work on this on my own and provide you a pull request, if you want.

terrischwartz commented 8 years ago

Hi Vincent,

That all sounds great. I haven't used TravisCI (or other CI envs). I'm looking forward to seeing how that works.

I'm swamped for the next few weeks so please forgive me if I'm slow to respond at times.

Terri


From: Vincent Vieira [notifications@github.com] Sent: Tuesday, August 16, 2016 9:39 AM To: terrischwartz/tus_servlet Cc: Schwartz, Terri; Comment Subject: Re: [terrischwartz/tus_servlet] Complete container coverage (#1)

Terri,

About the packaging, and if you're ok with it, I'll create a specific issue covering the main issue and the possible workarounds in order to do better.

About the NIO exception, I used the official Java client to upload a tiny file of 1.5kB, so yes.

About tests, I suggest that we keep your tests. Those bash scripts can be run in a CI environment (that would be also great, using TravisCI for instance) or through a custom Maven execution when running tests.

In order to cover in a more comprehensive way all containers, I suggest that we update the module's POM in order to include all latest embedded containers (with a test scope of course) and run a test for each one of them. I'm sure that Jetty, Tomcat and Undertow are provided out of the box. I'm 80% sure that JBoss and Glassfish can be run in tests using the EmbeddedEJBContainerhttp://docs.oracle.com/javaee/6/api/javax/ejb/embeddable/EJBContainer.html#createEJBContainer(), this container will be simply configured as a Servlet one.

I can work on this on my own and provide you a pull request, if you want.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/terrischwartz/tus_servlet/issues/1#issuecomment-240160855, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AKrpNOVdtxHYSL4E4RVewhj_LptwjTyoks5qgee4gaJpZM4Jlh1G.

vincent-vieira commented 8 years ago

No problem ! I'll do this work on my own and list you all the issues I found on other topics. This one will be only dedicated to better container support.