You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/06/06 17:28:00 UTC
[GitHub] [zookeeper] chevaris opened a new pull request #1373: ZOOKEEPER-3803
chevaris opened a new pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373
Prevent NPE on TestingServer close()
When using TestingServer, close method triggers an unneed call to fastForwardFromEdits on FileTxnSnapLog instance that has been already closed throwing a NPE that is harmless but a bit annoying on development
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1373: ZOOKEEPER-3803
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373#issuecomment-643580519
Sorry for late reply. I forgot to push the button in github
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] chevaris edited a comment on pull request #1373: ZOOKEEPER-3803
Posted by GitBox <gi...@apache.org>.
chevaris edited a comment on pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373#issuecomment-643604080
Thx a lot Enrico. I agree with you that the fix I provided it is not elegant (it was suggested in the ticket)
I have checked the code again:
- All the code relies on the fact variable is not null (variable is initializated at construction), although there are some caveats (see next bullet). Variablke is never check against null in the class to avoid NPE, so the fix only follows general approach of the code.
- FileTxnSnapLog.truncateLog calls the close() method setting txnLog to null, but later resets the txnLog with a new instance, so in between all the calls that involve txnLog are exposed to NPE. Is it expected that class is called from multiple threads? Because if that is the case, synchronization is missing
- A Closeable class as a general rule SHOULD NOT be used after closed, but class is not protected to that (even internally the close method is called internally). Actually when the method is called internally there are side effects as commented in pevious bullet
- This class looks is accessed from multiple threads (can you confirm?), but for instance txnLog variable is not safely publish (e.g. as volatile) or synchronized at all.
I do not have exprience with ZK internals, but looks to me that there are some weird race conditions on shutdown (or when TX log is truncated) that prevents the flow to run smoothly and at least in my preliminar investigation I do not see a clear way out. Could you provide me a hint how to continue? I do not see how the problem can be fixed in the shutdown method (or only there)
Thanks in advance
/Chevarius
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] chevaris edited a comment on pull request #1373: ZOOKEEPER-3803
Posted by GitBox <gi...@apache.org>.
chevaris edited a comment on pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373#issuecomment-643604080
Thx a lot Enrico. I agree with you that the fix I provided it is not elegant (it was suggested in the ticket)
I have checked the code again and I see race conditions with txnLog variable
- FileTxnSnapLog.truncateLog calls the close() method setting txnLog to null, but later resets the txnLog with a new instance, so in between all the calls that involve txnLog are exposed to NPE
- A Closeable class as a general rule SHOULD NOT be used after closed, but class is not protected to that (even internally the close method is called)
- This class looks is accessed from multiple threads, but for instance txnLog variable is not safely publish (e.g. as volatile) or synchronized at all.
I do not have exprience with ZK internals, but looks to me that there are some weird race conditions on shutdown (or when TX log is truncated) that prevents the flow to run smoothly and at least in my preliminar investigation I do not see a clear way out. Could you provide me a hint how to continue? I do not see how the problem can be fixed in the shutdown method (or only there)
Thanks in advance
/Evaristo
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1373: ZOOKEEPER-3803 Prevent NPE on TestingServer close()
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373#issuecomment-711489711
Should we close this pull request given the appertained JIRA ticket ZOOKEEPER-3803 is closed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1373: ZOOKEEPER-3803 Prevent NPE on TestingServer close()
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373#issuecomment-648633149
I added a comment to the JIRA
https://issues.apache.org/jira/browse/ZOOKEEPER-3803
it is better to fix this problem here in ZooKeeper
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] chevaris commented on pull request #1373: ZOOKEEPER-3803
Posted by GitBox <gi...@apache.org>.
chevaris commented on pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373#issuecomment-640193142
Adjusting commit message to requested format
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli closed pull request #1373: ZOOKEEPER-3803 Prevent NPE on TestingServer close()
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] chevaris commented on pull request #1373: ZOOKEEPER-3803
Posted by GitBox <gi...@apache.org>.
chevaris commented on pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373#issuecomment-643604080
Thx a lot Enrico. I agree with you that the fix I provided it is not elegant (it was suggested in the ticket)
I have checked the code again and I see race conditions with txnLog variable
- FileTxnSnapLog.truncateLog calls the close() method setting txnLog to null, but later resets the txnLog with a new instance, so in between all the calls that involve txnLog are exposed to NPE
- A Closeable class as a general rule SHOULD NOT be used after closed, but class is not protected to that (even internally the close method is called)
- This class looks is accessed from multiple threads, but for instance txnLog variable is not safely publish (e.g. as volatile) or synchronized at all.
I do not have exprience with ZK internals, but looks to me that there are some weird race conditions on shutdown (or when TX log is truncated) that prevents the flow to run smoothly and at least in my preliminar investigation I do not see a clear way out. Could you provide me a hint how to continue? I do not see how the problem can be fixed in the shutdown method (or only there)
Thanks in advance
/Evaristo
I think the code has some race conditions and lacks some synchronization
FileTxnSnapLog,
f you check the code 'txnLog' can be null only when the instance is closed (I assume to allow GC to collect memory). I assume that once instance is closed FileTxnSnapLog.java methods should not be called, right? The class is missing that protection
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1373: ZOOKEEPER-3803 Prevent NPE on TestingServer close()
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1373:
URL: https://github.com/apache/zookeeper/pull/1373#issuecomment-711797077
@hanm you are right
@chevaris feel free to reopen the ticket
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org