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

Add CR in sanity check #2468

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@BiNZGi
Copy link
Contributor

BiNZGi commented Dec 18, 2018

Specified in https://stomp.github.io/stomp-specification-1.2.html#STOMP_Frames also carriage return (octet 13) is allowed:

The frame starts with a command string terminated by an end-of-line (EOL), which consists of an OPTIONAL carriage return (octet 13) followed by a REQUIRED line feed (octet 10).

Add CR in sanity check
Specified in https://stomp.github.io/stomp-specification-1.2.html#STOMP_Frames also carriage return (octet 13) is allowed.
@jbertram

This comment has been minimized.

Copy link
Contributor

jbertram commented Dec 18, 2018

Could you create a JIRA for this at https://issues.apache.org/jira/projects/ARTEMIS and also add a test to reproduce the failure you were seeing without the fix? Something simple in org.apache.activemq.artemis.tests.integration.stomp.StompTest should suffice.

@jbertram

This comment has been minimized.

Copy link
Contributor

jbertram commented Dec 19, 2018

After looking at this a bit more closely I think this change is incorrect. The optional carriage return (octet 13) was added in STOMP 1.2, but you have changed org.apache.activemq.artemis.core.protocol.stomp.StompDecoder which is responsible for STOMP 1.0 frames. There are different decoder implementations for STOMP 1.1 & 1.2, and from what I can tell the 1.2 decoder handles the EOL properly. In fact, the STOMP client used by the test suite sends \r\n EOLs by default when using STOMP 1.2.

Unless you have a test demonstrating a problem I think this PR should be closed.

@BiNZGi

This comment has been minimized.

Copy link
Contributor Author

BiNZGi commented Dec 19, 2018

Thank you for the hint with JIRA. Today I have made some more tests and recognized that there was a mismatch with the STOMP versions in my case. The messages were in STOMP 1.0 but with CR that is not allowed in this version.
Therefore I close this PR.

@BiNZGi BiNZGi closed this Dec 19, 2018

@BiNZGi BiNZGi deleted the BiNZGi:patch-2 branch Dec 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.