-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(jsonrpc): enforce log filter cap and improve match efficiency #6732
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
base: develop
Are you sure you want to change the base?
Changes from all commits
073771e
2c9b59d
440582b
0016f13
0b80861
6620708
5f03ddb
1b1a081
bf1f100
6dd2d44
9259d2b
46c9629
732d66a
938533a
5d15ea2
9493ecf
e99045d
ad0d688
a48b3b2
0f48bda
2666f98
2f2e11a
44266d6
481130d
7c4b941
a209e45
4bf108a
5ba9053
584b53e
0cce053
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,5 @@ | ||
| package org.tron.common.logsfilter.capsule; | ||
|
|
||
| public class FilterTriggerCapsule extends TriggerCapsule { | ||
| public class FilterTriggerCapsule { | ||
|
|
||
| public void processFilterTrigger() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ | |
| import org.apache.commons.collections4.CollectionUtils; | ||
| import org.bouncycastle.util.encoders.Hex; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.context.annotation.Lazy; | ||
| import org.springframework.stereotype.Component; | ||
| import org.tron.api.GrpcAPI; | ||
| import org.tron.api.GrpcAPI.TransactionInfoList; | ||
|
|
@@ -142,6 +143,7 @@ | |
| import org.tron.core.service.MortgageService; | ||
| import org.tron.core.service.RewardViCalService; | ||
| import org.tron.core.services.event.exception.EventException; | ||
| import org.tron.core.services.jsonrpc.TronJsonRpcImpl; | ||
| import org.tron.core.store.AccountAssetStore; | ||
| import org.tron.core.store.AccountIdIndexStore; | ||
| import org.tron.core.store.AccountIndexStore; | ||
|
|
@@ -277,6 +279,10 @@ public class Manager { | |
| @Autowired | ||
| private RewardViCalService rewardViCalService; | ||
|
|
||
| @Lazy | ||
| @Autowired | ||
| private TronJsonRpcImpl tronJsonRpcImpl; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SHOULD] Avoid @lazy circular dependency from Manager into TronJsonRpcImpl
Why this matters even though it compiles:
Two options to resolve before merge: Option A — drop the reverse dependency (smallest change, matches the
Option C — Visitor / Dispatcher (cleanest, may warrant splitting from this PR):
Suggestion: pick A or C. If A, this PR is lighter than today; if C, the architecture is durable and the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very good suggestion. To keep durable,
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SummaryIt don't works if remove After removing Why The Proposed Dependency GraphAt first glance, all three edges point in one direction — no cycle. The Hidden Transitive Dependencies
@Autowired
public TronJsonRpcImpl(NodeInfoService nodeInfoService, Wallet wallet) { … }
Both of those beans carry a hard dependency back to Manager:
// NodeInfoService.java
@Autowired private Manager dbManager;
// Wallet.java
@Autowired private Manager dbManager;Expanding the full graph: Why Spring Rejects ThisFullNode.java explicitly disables circular-reference resolution: With this flag, Spring does not expose an early singleton reference for beans that are
The new class does not appear anywhere in the cycle; the cycle runs entirely through What Would Actually Fix ItThe FilterTriggerDispatcher design only eliminates the cycle if one of the following is Without one of these additions, introducing FilterTriggerDispatcher changes only the What to do next ?If a more systematic architectural refactor is planned in the future, FilterTriggerDispatcher is worth tracking in a separate issue and addressing together with changing NodeInfoService / Wallet to setter injection.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the detailed analysis — you are right that introducing But I think the conversation we're having so far is still framed at the wrong level. The three workarounds you listed (
The reason this dependency exists today is that Concrete plan: move
|
||
|
|
||
| /** | ||
| * Cycle thread to rePush Transactions | ||
| */ | ||
|
|
@@ -333,8 +339,10 @@ public class Manager { | |
| while (isRunFilterProcessThread) { | ||
| try { | ||
| FilterTriggerCapsule filterCapsule = filterCapsuleQueue.poll(1, TimeUnit.SECONDS); | ||
| if (filterCapsule != null) { | ||
| filterCapsule.processFilterTrigger(); | ||
| if (filterCapsule instanceof LogsFilterCapsule) { | ||
| tronJsonRpcImpl.handleLogsFilter((LogsFilterCapsule) filterCapsule); | ||
| } else if (filterCapsule instanceof BlockFilterCapsule) { | ||
| tronJsonRpcImpl.handleBLockFilter((BlockFilterCapsule) filterCapsule); | ||
| } | ||
|
317787106 marked this conversation as resolved.
|
||
| } catch (InterruptedException e) { | ||
| logger.error("FilterProcessLoop get InterruptedException, error is {}.", | ||
|
|
@@ -2279,7 +2287,8 @@ private void reOrgLogsFilter() { | |
| } | ||
|
|
||
| private void postBlockFilter(final BlockCapsule blockCapsule, boolean solidified) { | ||
| BlockFilterCapsule blockFilterCapsule = new BlockFilterCapsule(blockCapsule, solidified); | ||
| BlockFilterCapsule blockFilterCapsule = | ||
| new BlockFilterCapsule(blockCapsule, solidified); | ||
| if (!filterCapsuleQueue.offer(blockFilterCapsule)) { | ||
| logger.info("Too many filters, block filter lost: {}.", blockCapsule.getBlockId()); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[NIT] Add isDaemon overload to newForkJoinPool to match sibling factories
All the other factories in this class come in pairs —
newSingleThreadExecutor(name)/newSingleThreadExecutor(name, isDaemon), same fornewSingleThreadScheduledExecutorandnewFixedThreadPool. The newnewForkJoinPool(String, int)only has the single-arg form, with no way to opt into daemon threads.Suggestion: Add
newForkJoinPool(String name, int parallelism, boolean isDaemon)that callsthread.setDaemon(isDaemon)inside the worker factory; have the two-arg form delegate withisDaemon=false(matchingnewFixedThreadPool's default).Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add following in
ExecutorServiceManager:isDaemonisfalsedefault.