use original opcode in DynamicMethodBuilder #448
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lucaspimentel
added
type:bug
area:automatic-instrumentation
lucaspimentel
changed the title
lucaspimentel
force-pushed
the
lpimentel/use-original-opcode-everywhere
branch
2 times, most recently
from
920e17c to
bec0b10
Compare
reproductions/StackExchange.Redis.StackOverflow/StackExchange.Redis.StackOverflow.csproj
Show resolved
Hide resolved
src/Datadog.Trace.ClrProfiler.Managed/Integrations/StackExchange.Redis/RedisBatch.cs
Outdated
Show resolved
Hide resolved
lucaspimentel
force-pushed
the
lpimentel/use-original-opcode-everywhere
branch
2 times, most recently
from
c402644 to
29c84a5
Compare
lucaspimentel
force-pushed
the
lpimentel/use-original-opcode-everywhere
branch
from
29c84a5 to
40e8d59
Compare
lucaspimentel
force-pushed
the
lpimentel/use-original-opcode-everywhere
branch
from
74fdb67 to
929de65
Compare
lucaspimentel
commented
Jul 17, 2019
| methodParameterTypes: new[] { messageType, processorType, serverType }, | ||
| methodGenericArguments: new[] { genericType }); | ||
| .GetOrCreateMethodCallDelegate( | ||
| _redisBaseType, |
Member
Author
There was a problem hiding this comment.
NOTE: we intercepted a call to RedisBase.ExecuteAsync(), so we need to call that method, not the one in type redisBase.GetType(), which could be an override. This is required in addition to using the original opcode to avoid stack overflow exceptions (#447).
lucaspimentel
removed
the
status:work-in-progress
| m.GetGenericArguments().Length == methodGenericArguments.Length) | ||
| .ToArray(); | ||
| m => m.IsGenericMethodDefinition && | ||
| m.GetGenericArguments().Length == methodGenericArguments.Length); |
Contributor
There was a problem hiding this comment.
Good find, no need to generate a new array!
| { | ||
| // get these only once and cache them, | ||
| // no need for locking, race conditions are not a problem | ||
| _redisAssembly = thisType.Assembly; |
Contributor
There was a problem hiding this comment.
Nice! I feel like I should edit my GraphQL integration to do this since the GraphQL parameters are defined in the same assembly.
Member
Author
There was a problem hiding this comment.
I'm ok with doing it either way (like this or using the assembly name). In this case the assembly name can be either StackExchange.Redis or StackExchange.Redis.StrongName, so doing it this way allows us to handle either one.
zacharycmontoya
approved these changes
Jul 17, 2019
Contributor
zacharycmontoya
left a comment
zacharycmontoya
left a comment
There was a problem hiding this comment.
Looks good to me! 👍
lucaspimentel
force-pushed
the
lpimentel/use-original-opcode-everywhere
branch
from
e9c441a to
d936f73
Compare
lucaspimentel
force-pushed
the
lpimentel/use-original-opcode-everywhere
branch
from
d936f73 to
343381c
Compare
colin-higgins
approved these changes
Jul 18, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to #367.
Fixes the
StackOverflowExceptioninStackExchange.Redisintegration (see #447).Changes proposed in this pull request:
DynamicMethodBuilderStackExchange.Redisintegration, callRedisBase.ExecuteAsync()instead of theExecuteAsync()method from the derived typeHttpMessageHandler.StackOverflowproject toHttpMessageHandler.StackOverflowException