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

Accept large slim messages #504

Merged
merged 2 commits into from Aug 17, 2014

Conversation

Projects
None yet
4 participants
@raboof
Contributor

raboof commented Aug 13, 2014

raboof added some commits Jul 25, 2014

Allow for responses larger than 999999 characters
The protocol officially allows for 6-digit length headers, but in practice
some SlimServer implementations (like the Java one) may emit larger results.
Of course that's likely to cause problems elsewhere, but AFAICS there's no
reason we shouldn't be lenient about that.

Roughly this patch was earlier accepted as 0bbd3c4
and then reverted in e98714c, I don't remember
why...
@roboaks

This comment has been minimized.

Show comment
Hide comment
@roboaks

roboaks Jul 27, 2014

Will this fork support both requests and responses larger than 1Mb (we will certainly need that). If so, should I go ahead and test this fork? Or should we still wait for Arjan's response?

roboaks commented on 5bda9ee Jul 27, 2014

Will this fork support both requests and responses larger than 1Mb (we will certainly need that). If so, should I go ahead and test this fork? Or should we still wait for Arjan's response?

This comment has been minimized.

Show comment
Hide comment
@raboof

raboof Jul 27, 2014

Owner

Yes, this fork should support both large requests and large responses (though I haven't tested beyond the unittests, so there could still be some stuff missing). I'd still like Arjan's feedback before requesting those changes to be merged, but IIRC he's on vacation right now.

Perhaps you could take those changes for a spin in the mean time? I could provide a jar or standalone-jar to test if necessary.

Owner

raboof replied Jul 27, 2014

Yes, this fork should support both large requests and large responses (though I haven't tested beyond the unittests, so there could still be some stuff missing). I'd still like Arjan's feedback before requesting those changes to be merged, but IIRC he's on vacation right now.

Perhaps you could take those changes for a spin in the mean time? I could provide a jar or standalone-jar to test if necessary.

This comment has been minimized.

Show comment
Hide comment
@roboaks

roboaks Aug 4, 2014

Hey Arnout, I'm wondering if you generated the pull request in response to Arjan's comment on 0bbd3c4?

roboaks replied Aug 4, 2014

Hey Arnout, I'm wondering if you generated the pull request in response to Arjan's comment on 0bbd3c4?

This comment has been minimized.

Show comment
Hide comment
@raboof

raboof Aug 5, 2014

Owner

Sorry, I was waiting to see if you could provide a confirm the patches actually have the desired effect.

You mentioned in roboaks@5bda9ee#commitcomment-7187761 you ran into trouble compiling FitNesse from this branch. I uploaded one to http://we.tl/88mRmpIKBq for you. Looking forward to hearing about the results!

Owner

raboof replied Aug 5, 2014

Sorry, I was waiting to see if you could provide a confirm the patches actually have the desired effect.

You mentioned in roboaks@5bda9ee#commitcomment-7187761 you ran into trouble compiling FitNesse from this branch. I uploaded one to http://we.tl/88mRmpIKBq for you. Looking forward to hearing about the results!

This comment has been minimized.

Show comment
Hide comment
@roboaks

roboaks Aug 5, 2014

Sorry for the confusion Arnout. I'm not sure why the standalone jar I built didn't work...and then I had to leave for a client engagement. Thank you for providing the jar via wetransfer. I will test it in the next 24 hours and get back to you.

roboaks replied Aug 5, 2014

Sorry for the confusion Arnout. I'm not sure why the standalone jar I built didn't work...and then I had to leave for a client engagement. Thank you for providing the jar via wetransfer. I will test it in the next 24 hours and get back to you.

This comment has been minimized.

Show comment
Hide comment
@raboof

raboof Aug 10, 2014

Owner
!path %CommonRootPath%/xebium-0.12-SNAPSHOT-jar-with-dependencies.jar    
!path %CommonRootPath%/fitnesse-standalone.jar

Hmm. Could it be that xebium-0.12-SNAPSHOT-jar-with-depencencies is giving you the 'old' SlimServer? Could you try reversing the order of those 2 jars on the classpath?

Owner

raboof replied Aug 10, 2014

!path %CommonRootPath%/xebium-0.12-SNAPSHOT-jar-with-dependencies.jar    
!path %CommonRootPath%/fitnesse-standalone.jar

Hmm. Could it be that xebium-0.12-SNAPSHOT-jar-with-depencencies is giving you the 'old' SlimServer? Could you try reversing the order of those 2 jars on the classpath?

This comment has been minimized.

Show comment
Hide comment
@roboaks

roboaks Aug 10, 2014

My wife was somewhat alarmed when I shouted with joy 😃. That solved the problem Arnout. It was indeed the old slim classes embedded in the standalone Xebium jar. Well well done. I will do some further testing to confirm the resolution, but I am 99% sure we're good to go since it's one of those things that either works or doesn't work.

Enjoy your Sunday.

roboaks replied Aug 10, 2014

My wife was somewhat alarmed when I shouted with joy 😃. That solved the problem Arnout. It was indeed the old slim classes embedded in the standalone Xebium jar. Well well done. I will do some further testing to confirm the resolution, but I am 99% sure we're good to go since it's one of those things that either works or doesn't work.

Enjoy your Sunday.

This comment has been minimized.

Show comment
Hide comment
@raboof

raboof Aug 11, 2014

Owner

Good to hear :). I'll make it a pull request after your confirmation.

Owner

raboof replied Aug 11, 2014

Good to hear :). I'll make it a pull request after your confirmation.

This comment has been minimized.

Show comment
Hide comment
@roboaks

roboaks Aug 13, 2014

I am happy to report that everything seems to work fine. Please go ahead and make the pull request!

roboaks replied Aug 13, 2014

I am happy to report that everything seems to work fine. Please go ahead and make the pull request!

This comment has been minimized.

Show comment
Hide comment
@raboof

raboof Aug 13, 2014

Owner

Great!

Owner

raboof replied Aug 13, 2014

Great!

@woodybrood

This comment has been minimized.

Show comment
Hide comment
@woodybrood

woodybrood Aug 13, 2014

Collaborator

Has this commit been checked for interoperability with CSlim or some of the
other Slim servers to make sure they are not broken by it? Do we need to
bump the slim protocol version number?

On Wed, Aug 13, 2014 at 7:47 AM, Arnout Engelen notifications@github.com
wrote:

As tested by @roboaks https://github.com/roboaks, raboof/fitnesse@5bda9ee
#commitcomment-7371574

raboof@5bda9ee#commitcomment-7371574

You can merge this Pull Request by running

git pull https://github.com/raboof/fitnesse acceptLargeSlimMessages

Or view, comment on, or merge it at:

#504
Commit Summary

  • Allow for responses larger than 999999 characters
  • Accept large instructions from the Slim client

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#504.

Collaborator

woodybrood commented Aug 13, 2014

Has this commit been checked for interoperability with CSlim or some of the
other Slim servers to make sure they are not broken by it? Do we need to
bump the slim protocol version number?

On Wed, Aug 13, 2014 at 7:47 AM, Arnout Engelen notifications@github.com
wrote:

As tested by @roboaks https://github.com/roboaks, raboof/fitnesse@5bda9ee
#commitcomment-7371574

raboof@5bda9ee#commitcomment-7371574

You can merge this Pull Request by running

git pull https://github.com/raboof/fitnesse acceptLargeSlimMessages

Or view, comment on, or merge it at:

#504
Commit Summary

  • Allow for responses larger than 999999 characters
  • Accept large instructions from the Slim client

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#504.

@raboof

This comment has been minimized.

Show comment
Hide comment
@raboof

raboof Aug 13, 2014

Contributor
  • 15a8853 is a change to the SlimServer, so that can't impact CSlim.
  • 5bda9ee makes FitNesse more lenient in what it accepts from the Slim side, so that can't negatively impact CSlim.

I haven't actually tested with CSlim, but this makes me confident that shouldn't break.

Contributor

raboof commented Aug 13, 2014

  • 15a8853 is a change to the SlimServer, so that can't impact CSlim.
  • 5bda9ee makes FitNesse more lenient in what it accepts from the Slim side, so that can't negatively impact CSlim.

I haven't actually tested with CSlim, but this makes me confident that shouldn't break.

@woodybrood

This comment has been minimized.

Show comment
Hide comment
@woodybrood

woodybrood Aug 13, 2014

Collaborator

OK. That makes sense. Just wanted to confirm.

On Wed, Aug 13, 2014 at 9:50 AM, Arnout Engelen notifications@github.com
wrote:

  • 15a8853
    15a8853
    is a change to the SlimServer, so that can't impact CSlim.
  • 5bda9ee
    5bda9ee
    makes FitNesse more lenient in what it accepts from the Slim side, so that
    can't negatively impact CSlim.

I haven't actually tested with CSlim, but this makes me confident that
shouldn't break.


Reply to this email directly or view it on GitHub
#504 (comment).

Collaborator

woodybrood commented Aug 13, 2014

OK. That makes sense. Just wanted to confirm.

On Wed, Aug 13, 2014 at 9:50 AM, Arnout Engelen notifications@github.com
wrote:

  • 15a8853
    15a8853
    is a change to the SlimServer, so that can't impact CSlim.
  • 5bda9ee
    5bda9ee
    makes FitNesse more lenient in what it accepts from the Slim side, so that
    can't negatively impact CSlim.

I haven't actually tested with CSlim, but this makes me confident that
shouldn't break.


Reply to this email directly or view it on GitHub
#504 (comment).

@raboof

This comment has been minimized.

Show comment
Hide comment
@raboof

raboof Aug 13, 2014

Contributor

So we don't need to bump the slim protocol version, as the changes are backwards-compatible. We could do it, of course: this would make the improvements 'officially part of the slim api' and clients could make sure they support.

It's quite a corner case though, so I'm not sure it'd be worth it...

Contributor

raboof commented Aug 13, 2014

So we don't need to bump the slim protocol version, as the changes are backwards-compatible. We could do it, of course: this would make the improvements 'officially part of the slim api' and clients could make sure they support.

It's quite a corner case though, so I'm not sure it'd be worth it...

@amolenaar

This comment has been minimized.

Show comment
Hide comment
@amolenaar

amolenaar Aug 14, 2014

Collaborator

I think a version bump is not needed. We just got a little lenient on handling sizes that exceed 6 digits (which could already be generated apparently).

Collaborator

amolenaar commented Aug 14, 2014

I think a version bump is not needed. We just got a little lenient on handling sizes that exceed 6 digits (which could already be generated apparently).

@amolenaar amolenaar added this to the Next release milestone Aug 14, 2014

@amolenaar amolenaar merged commit 5bda9ee into unclebob:master Aug 17, 2014

amolenaar added a commit that referenced this pull request Aug 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment