You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2021/12/02 10:32:42 UTC
[GitHub] [ratis] guohao-rosicky opened a new pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
guohao-rosicky opened a new pull request #549:
URL: https://github.com/apache/ratis/pull/549
## What changes were proposed in this pull request?
NettyServerStreamRpc get request resolving concurrency security
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1454
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985177341
@guohao-rosicky , thanks, I read these netty API docs earlier. Some questions:
- Is there a problem you observed? If yes, could you show the datails?
- The calls might have shared the same context. We need to verify it.
- Somehow the attr method ( https://netty.io/4.1/api/io/netty/channel/ChannelHandlerContext.html#attr-io.netty.util.AttributeKey- ) is deprecated. So, it is not clear what is the correct way to use it.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky closed pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky closed pull request #549:
URL: https://github.com/apache/ratis/pull/549
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985274189
> There is a potential problem, ...
The current code does not seem to have a potential problem since we have assertions in the set(..) and reset(..) methods in RequestRef.
If there is no problem observed, let's don't spend our energy to change this. We have many other works going on.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985355635
> ... causing an error message to be sent to other clients
It seems not possible since requestRef already has asserted that only one request is process at a time in set/reset methods.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985252483
I will test in my test environment for a period of time and then update the test progress
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-984511487
@szetszwo , can you help review this patch?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985169298
> ... you can see from netty document ...
Could you provide the link to the netty doc?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky edited a comment on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky edited a comment on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985330365
```
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable throwable) {
Optional.ofNullable(requestRef.getAndSetNull())
.ifPresent(request -> requests.replyDataStreamException(throwable, request, ctx));
}
```
@szetszwo
`requestRef.getAndSetNull()`Before calling, it is modified by other `ChannelPipeline`, causing an error message to be sent to other clients
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985370576
>
But won't this variable be modified by other channels before calling exceptionCaught in this ChannelPipeline? Set /reset is only guaranteed to remain unchanged within the channelRead method in this ChannelPipeline.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985167562
@szetszwo , I didn't find any exceptions, but you can see from netty document can do the same thing with AttributeMap.
```
private Final RequestRef RequestRef = new RequestRef();
```
changed by another `ChannelPipeline`, the `exceptionCaught` method obtained an incorrect value.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky edited a comment on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky edited a comment on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985203931
> @guohao-rosicky , thanks, I read these netty API docs earlier. Some questions:
>
> * Is there a problem you observed? If yes, could you show the datails?
> * The calls might have shared the same context. We need to verify it.
> * Somehow the attr method ( https://netty.io/4.1/api/io/netty/channel/ChannelHandlerContext.html#attr-io.netty.util.AttributeKey- ) is deprecated. So, it is not clear what is the correct way to use it.
@szetszwo
1. There is a potential problem, but it has not been discovered
2. I used the `ChannelHandlerContext.name ()` this is the only name each context
3. I use `ctx.channel().attr()` and will not deprecated this method
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985330365
```
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable throwable) {
Optional.ofNullable(requestRef.getAndSetNull())
.ifPresent(request -> requests.replyDataStreamException(throwable, request, ctx));
}
```
@szetszwo
`requestRef.getAndSetNull()`Before calling, it is modified by other channel pipelines, causing an error message to be sent to other clients
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky edited a comment on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky edited a comment on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985203931
> @guohao-rosicky , thanks, I read these netty API docs earlier. Some questions:
>
> * Is there a problem you observed? If yes, could you show the datails?
> * The calls might have shared the same context. We need to verify it.
> * Somehow the attr method ( https://netty.io/4.1/api/io/netty/channel/ChannelHandlerContext.html#attr-io.netty.util.AttributeKey- ) is deprecated. So, it is not clear what is the correct way to use it.
@szetszwo
1. There is a potential problem, but it has not been discovered
2. I used the `ChannelHandlerContext.name()` this is the only name each context, `AttributeKey` is obtained by `name()` and is unique in each context, not overwritten by other handles
3. I use `ctx.channel().attr()` and will not deprecated this method
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985203931
> @guohao-rosicky , thanks, I read these netty API docs earlier. Some questions:
>
> * Is there a problem you observed? If yes, could you show the datails?
> * The calls might have shared the same context. We need to verify it.
> * Somehow the attr method ( https://netty.io/4.1/api/io/netty/channel/ChannelHandlerContext.html#attr-io.netty.util.AttributeKey- ) is deprecated. So, it is not clear what is the correct way to use it.
1. There is a potential problem, but it has not been discovered
2. I used the `ChannelHandlerContext.name ()` this is the only name each context
3. I use `ctx.channel().attr()` and will not deprecated this method
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985142294
@guohao-rosicky , Could you describe the details of the problem in more details? Did you see any exceptions?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985171320
> > ... you can see from netty document ...
>
> Could you provide the link to the netty doc?
doc:
https://netty.io/4.1/api/io/netty/util/AttributeMap.html
https://netty.io/4.1/api/io/netty/util/AttributeKey.html
https://netty.io/4.1/api/io/netty/channel/ChannelHandlerContext.html#name--
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-985983274
@guohao-rosicky , only set/reset methods can change the ref. Suppose ref is set to requestA. Only reset(requestA) can clear it. There are no ways (even in other methods) to set it to another request before that.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky commented on pull request #549: RATIS-1454. NettyServerStreamRpc get request resolving concurrency se…
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #549:
URL: https://github.com/apache/ratis/pull/549#issuecomment-984499768
netty doc: https://netty.io/4.1/api/io/netty/util/AttributeKey.html
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org