You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/05/09 06:05:33 UTC

[GitHub] [rocketmq] zhanxuefeng opened a new pull request, #4266: [ISSUE #4265] Fix query message get the wrong result when tls enabled

zhanxuefeng opened a new pull request, #4266:
URL: https://github.com/apache/rocketmq/pull/4266

   **Make sure set the target branch to `develop`**
   
   ## What is the purpose of the change
   
   Fix query message get the wrong result when tls enabled
   
   ## Brief changelog
   
   Correct the return value of WritableByteChannel's write method in FileRegionEncoder
   


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] zhanxuefeng commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
zhanxuefeng commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1124448228

   @hzh0425
   The same logs mentioned in [ISSUE #4265](https://github.com/apache/rocketmq/issues/4265)
   
   ### FileRegionEncoder
   ![image](https://user-images.githubusercontent.com/19700253/167974729-aca1b14d-8294-4c10-a0d1-1952a0e3fbb8.png)
   
   I search the same msg mentioned in [ISSUE #4265](https://github.com/apache/rocketmq/issues/4265)
   ![image](https://user-images.githubusercontent.com/19700253/167974840-44673b2a-4d8f-406a-9403-45b3c16c60eb.png)
   Header is 193 bytes and transferred 193 bytes, the 1st msg is 378 bytes and transferred 378 bytes, the 2nd msg is 450 bytes and transferred 450 bytes, total 1021 bytes transferred. 
   ![image](https://user-images.githubusercontent.com/19700253/167975064-edcca0e7-396e-48f7-be95-0615a6311cbb.png)
   NettyDecoder get 1021 bytes and show the sub trace and pub trace.
   


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] zhanxuefeng commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
zhanxuefeng commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1121041485

   > Can you create an issue to describe this?
   
   [ISSUE #4266](https://github.com/apache/rocketmq/issues/4265) describe the bug.


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] hzh0425 commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
hzh0425 commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1123869788

   @zhanxuefeng oh, I understand, can you add a test to prove your pr?


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] zhanxuefeng commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
zhanxuefeng commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1125618437

   Prove `ManyMessageProcessor`
   
   With my code changed in `FileRegionEncoder`
   ![image](https://user-images.githubusercontent.com/19700253/168204301-cdd8bbbf-010a-4b42-86f3-ac1c924b517e.png)
   ### Console log
   ![image](https://user-images.githubusercontent.com/19700253/168204772-f7e69bf6-f00d-4f48-8cd1-2d1eb23fcf34.png)
   ### Consumer log
   ![image](https://user-images.githubusercontent.com/19700253/168204494-94819af9-7bf3-4ca5-bbf5-54a66507fd96.png)
   
   One header and 2 msgs were transferred, and consumer received 610 bytes, decode to 2 msgs.


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] hzh0425 commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
hzh0425 commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1120946871

   Can you create an issue to describe this?


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] MatrixHB commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
MatrixHB commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1124770759

   Good catch for this issue. 
   
   > ByteBuffer's capacity is always greater than it's readable bytes
   
   However, this bug involves `QueryMessageProcessor`, which is core function for RocketMQ. I wonder why this bug has never been found. Does it only occur when TLS enabled ?
   
   Your code change in `FileRegionEncoder` is very underlying. Does it affect other upper layer functions？ 


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] MatrixHB commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
MatrixHB commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1124766452

   Good catch for this issue. 
   
   However, this bug involves `QueryMessageProcessor`, which is core function for RocketMQ. I wonder why this bug has never been found. Does it only occur when TLS enabled?
   
   Your code change in `FileRegionEncoder` is very underlying. Could you assess the impact on other upper layer functions？ 


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] zhanxuefeng commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
zhanxuefeng commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1125612366

   Test `ManyMessageTransfer`
   
   RocketMQ version: 4.9.3
   tls.server.mode: enforcing
   transferMsgByHeap=false   this is very important
   
   The topic I created has 1 writeQueueNums and 1 readQueueNums, and I send 2 msgs to the topic.
   
   
   I add some logs.
   ### PullMessageProcessor
   ![image](https://user-images.githubusercontent.com/19700253/168202199-dacd4db7-5970-4b39-9099-2415186ff72f.png)
   ### ManyMessageProcessor
   ![image](https://user-images.githubusercontent.com/19700253/168202273-92f4ca48-a9a9-4b30-9e9c-b365bb675eca.png)
   
   ### A simple pull consumer
   ![image](https://user-images.githubusercontent.com/19700253/168202418-7344e657-0aee-4fc8-8b5e-65571c1a5840.png)
   
   ### Console log
   ![image](https://user-images.githubusercontent.com/19700253/168202506-e6b18cde-f1bd-4eeb-94b5-bb05b0be70b2.png)
   ### Consume log
   ![image](https://user-images.githubusercontent.com/19700253/168202890-b388b2d7-97b8-4289-a182-025c1a18d084.png)
   
   Similar to [ISSUE #4265](https://github.com/apache/rocketmq/issues/4265)
   610 bytes and 2 msgs was found, but only transferred 413(216 header + 197 1st msg) bytes, because 768(256 capacity + 512 capacity) > 610, so the 2nd msg will not be transferred.
   The consume get 413 but 610(4 length + 606 data) needed, so not msg received.
   
   


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] zhanxuefeng commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
zhanxuefeng commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1125604426

   @MatrixHB
   Yes, the bug only occur when tls enabled, `FIleRegionEncoder` will be add to `NettyRemotingServer`'s ChannelPipline only when tlsModel is enforcing.
   
   ### HandshakeHandler
   ![image](https://user-images.githubusercontent.com/19700253/168200261-38252b6c-f596-46d4-9372-48564b1f7b69.png)
   
   
   `ManyMessageTransfer`, `OneMessageTransfer` and `QueryMessageTransfer` are extend from `FileRegion` in RocketMQ
   
   `OneMessageTransfer` may not occur because it has a header and only one ByteBuffer.
   
   But `ManyMessageTransfer` has the problem too, which involves `PullMessageProcessor`, I'll show the test later.
   


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] codecov-commenter commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1124398109

   # [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4266?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#4266](https://codecov.io/gh/apache/rocketmq/pull/4266?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (35e6e43) into [develop](https://codecov.io/gh/apache/rocketmq/commit/446b76b07b4336ec720b518ef2e9964bff650934?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (446b76b) will **increase** coverage by `0.46%`.
   > The diff coverage is `100.00%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             develop    #4266      +/-   ##
   =============================================
   + Coverage      47.61%   48.07%   +0.46%     
   - Complexity      4958     5068     +110     
   =============================================
     Files            633      642       +9     
     Lines          42577    42781     +204     
     Branches        5589     5599      +10     
   =============================================
   + Hits           20273    20569     +296     
   + Misses         19796    19696     -100     
   - Partials        2508     2516       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/4266?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...che/rocketmq/remoting/netty/FileRegionEncoder.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL25ldHR5L0ZpbGVSZWdpb25FbmNvZGVyLmphdmE=) | `92.85% <100.00%> (+0.54%)` | :arrow_up: |
   | [...broker/processor/AbstractSendMessageProcessor.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvcHJvY2Vzc29yL0Fic3RyYWN0U2VuZE1lc3NhZ2VQcm9jZXNzb3IuamF2YQ==) | `40.62% <0.00%> (-8.68%)` | :arrow_down: |
   | [...he/rocketmq/remoting/protocol/RemotingCommand.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL3Byb3RvY29sL1JlbW90aW5nQ29tbWFuZC5qYXZh) | `72.45% <0.00%> (-6.18%)` | :arrow_down: |
   | [...in/java/org/apache/rocketmq/test/util/MQAdmin.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dGVzdC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvdGVzdC91dGlsL01RQWRtaW4uamF2YQ==) | `38.88% <0.00%> (-5.56%)` | :arrow_down: |
   | [...apache/rocketmq/remoting/netty/ResponseFuture.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL25ldHR5L1Jlc3BvbnNlRnV0dXJlLmphdmE=) | `85.00% <0.00%> (-5.00%)` | :arrow_down: |
   | [...g/apache/rocketmq/remoting/netty/NettyEncoder.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL25ldHR5L05ldHR5RW5jb2Rlci5qYXZh) | `46.15% <0.00%> (-3.85%)` | :arrow_down: |
   | [...rocketmq/remoting/netty/NettyRemotingAbstract.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL25ldHR5L05ldHR5UmVtb3RpbmdBYnN0cmFjdC5qYXZh) | `47.08% <0.00%> (-3.84%)` | :arrow_down: |
   | [...ava/org/apache/rocketmq/filter/util/BitsArray.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZmlsdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9maWx0ZXIvdXRpbC9CaXRzQXJyYXkuamF2YQ==) | `59.82% <0.00%> (-2.57%)` | :arrow_down: |
   | [...va/org/apache/rocketmq/logging/inner/Appender.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9BcHBlbmRlci5qYXZh) | `34.83% <0.00%> (-2.25%)` | :arrow_down: |
   | [...n/java/org/apache/rocketmq/store/RunningFlags.java](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL1J1bm5pbmdGbGFncy5qYXZh) | `31.11% <0.00%> (-2.23%)` | :arrow_down: |
   | ... and [76 more](https://codecov.io/gh/apache/rocketmq/pull/4266/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/rocketmq/pull/4266?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4266?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [446b76b...35e6e43](https://codecov.io/gh/apache/rocketmq/pull/4266?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] coveralls commented on pull request #4266: [ISSUE #4265] Fix query message get wrong result when tls enabled

Posted by GitBox <gi...@apache.org>.
coveralls commented on PR #4266:
URL: https://github.com/apache/rocketmq/pull/4266#issuecomment-1135737150

   
   [![Coverage Status](https://coveralls.io/builds/49046332/badge)](https://coveralls.io/builds/49046332)
   
   Coverage increased (+0.03%) to 52.145% when pulling **35e6e4314262851c51e7507ce23f2f8601eb3a7d on zhanxuefeng:develop** into **5d2bf3573c683eb08f028b3a6496d1cbfe1af33b on apache:develop**.
   


-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org