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

Update DubboCodec.java #3568

Closed
wants to merge 2 commits into from
Closed

Update DubboCodec.java #3568

wants to merge 2 commits into from

Conversation

linhuaichuan
Copy link

fix issue #3537: 新旧版本兼容问题

Brief changelog

dubbo 反序列化对分片流操作时的兼容性支持

Verifying this change

1、DubboCodec 解码时,对is流进行多次操作

            ObjectInput in = CodecSupport.deserialize(channel.getUrl(), is, proto);
            if (status == Response.OK) {
                Object data;
                if (res.isHeartbeat()) {
                    data = decodeHeartbeatData(channel, in);
                } else if (res.isEvent()) {
                    data = decodeEventData(channel, in);
                } else {
                    DecodeableRpcResult result;
                    if (channel.getUrl().getParameter(
                            Constants.DECODE_IN_IO_THREAD_KEY,
                            Constants.DEFAULT_DECODE_IN_IO_THREAD)) {
                        result = new DecodeableRpcResult(channel, res, is,
                                (Invocation) getRequestData(id), proto);
                        result.decode();
                    } else {
                        result = new DecodeableRpcResult(channel, res,
                                new UnsafeByteArrayInputStream(readMessageData(is)),
                                (Invocation) getRequestData(id), proto);
                    }
                    data = result;
                }
                res.setResult(data);

2、当序列化工具使用分片读取is时,将造成result.decode()解码错误;比如fst.

fix issue apache#3537: 新旧版本兼容问题
@codecov-io
Copy link

codecov-io commented Feb 26, 2019

Codecov Report

Merging #3568 into 2.6.x will decrease coverage by 0.07%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.6.x    #3568      +/-   ##
============================================
- Coverage     47.12%   47.04%   -0.08%     
+ Complexity     4392     4389       -3     
============================================
  Files           565      565              
  Lines         25088    25091       +3     
  Branches       4422     4422              
============================================
- Hits          11823    11805      -18     
- Misses        11436    11458      +22     
+ Partials       1829     1828       -1
Impacted Files Coverage Δ Complexity Δ
...m/alibaba/dubbo/rpc/protocol/dubbo/DubboCodec.java 58.41% <33.33%> (-1.79%) 13 <0> (ø)
...aba/dubbo/remoting/transport/mina/MinaChannel.java 42.25% <0%> (-11.27%) 15% <0%> (-1%)
.../remoting/transport/netty4/NettyServerHandler.java 72.22% <0%> (-11.12%) 6% <0%> (-1%)
...om/alibaba/dubbo/rpc/filter/ActiveLimitFilter.java 77.77% <0%> (-5.56%) 5% <0%> (-1%)
...a/dubbo/remoting/transport/netty/NettyChannel.java 61.25% <0%> (-5%) 20% <0%> (-1%)
.../com/alibaba/dubbo/monitor/dubbo/DubboMonitor.java 87.85% <0%> (-1.87%) 15% <0%> (ø)
...a/dubbo/remoting/transport/netty4/NettyClient.java 64.4% <0%> (+1.69%) 11% <0%> (+1%) ⬆️
...onfig/spring/extension/SpringExtensionFactory.java 78.37% <0%> (+2.7%) 9% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef2cae1...f3ac746. Read the comment docs.

@lovepoem
Copy link
Member

@linhuaichuan Can you please add some unit test or integration test to verify the change ?

@linhuaichuan
Copy link
Author

linhuaichuan commented Feb 27, 2019

@lovepoem
1、CodecSupport.deserialize 反序列化 is ,当序列化工具使用分片读流时,若RpcResult的序列化byte大于分片值时,将引起is的startIndex变动。比如fst工具:org.nustaq.serialization.util.FSTInputStream 默认使用 8000 byte 分片读取is。

2、result.decode() 这时在反序列is。若startIndex产生变动,将引起RpcResult 读取到的序列化byte不正确。最终可能产生 #3537 issue。

[DUBBO-3243] Fix Invalid use of BasicClientConnManager: connection st…
@limingwei
Copy link

哇哈哈
所以
要解决了么?
什么时候发?

@beiwei30
Copy link
Member

beiwei30 commented May 8, 2019

this change looks reasonable, but it will not solve issue #3537.

@beiwei30
Copy link
Member

beiwei30 commented May 8, 2019

I will port your change into main trunk with this: #3991. Once it's in, I will merge your change too. @linhuaichuan

chickenlj pushed a commit that referenced this pull request May 10, 2019
@caojiele caojiele mentioned this pull request May 10, 2019
6 tasks
@qixiaobo
Copy link
Contributor

qixiaobo commented Jul 20, 2019

The same issue with #4612
Why nobody takes care of old stable version like 2.6.x?

@qixiaobo
Copy link
Contributor

Another place also has this code
Why not modify all?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@AlbumenJ
Copy link
Member

@linhuaichuan hi, thanks for your contribution

pls resolve conflicts with the latest master branch

@AlbumenJ AlbumenJ added the status/waiting-for-feedback Need reporters to triage label Mar 15, 2021
@AlbumenJ
Copy link
Member

AlbumenJ commented Apr 6, 2021

Close for long time no response.
Please feel free to reopen if you have any question.

@AlbumenJ AlbumenJ closed this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/waiting-for-feedback Need reporters to triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.