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.

Sign up for GitHub

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

Jump to bottom

Save head encoding on sanitized String(s) #492

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Save head encoding on sanitized String(s) #492

merged 1 commit into from
Nov 15, 2024

Conversation

franz1981
Copy link
Contributor

In theory not just UTF-8, but UTF-16 as well is not needed to be checked (see https://stackoverflow.com/a/75072043)

Right now it seems getCharset is guarded by a lock despite being volatile; which is happening in the hot path and we could avoid it.

I've added another change to avoid string concat for a pretty common case i.e. empty head.

@franz1981 franz1981 requested a review from jamezp as a code owner November 14, 2024 14:13
@franz1981
Copy link
Contributor Author

I'm concerned by these changes @dmlloyd @jamezp because I see no tests are entering in the validation branch there, because head is null.

@dmlloyd
Copy link
Member

dmlloyd commented Nov 14, 2024

Right now it seems getCharset is guarded by a lock despite being volatile; which is happening in the hot path and we could avoid it.

This is a bug IMO. AFAICT, there's no possible sequence of code where the lock changes the atomicity semantics of reading this field compared to a plain volatile read.

@franz1981
Copy link
Contributor Author

@dmlloyd this is what you meant? see f7eebdf

dmlloyd
dmlloyd approved these changes Nov 14, 2024
View reviewed changes
Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The other things I've commented on are less important.

@franz1981
Copy link
Contributor Author

@jamezp ready to go? Let me know if you have other concerns

@jamezp jamezp merged commit 8334439 into jboss-logging:main Nov 15, 2024
3 checks passed
@franz1981 franz1981 deleted the small_improvements branch November 19, 2024 14:14
benkard pushed a commit to benkard/quarkus-googlecloud-jsonlogging that referenced this pull request Nov 27, 2024
chore(deps): update all non-major dependencies (mulk/quarkus-googlecl…
ca98285 
…oud-jsonlogging!25)

This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io.quarkus:quarkus-extension-processor](https://github.com/quarkusio/quarkus) |  | minor | `3.16.3` -> `3.17.0` |
| [io.quarkus:quarkus-extension-maven-plugin](https://github.com/quarkusio/quarkus) | build | minor | `3.16.3` -> `3.17.0` |
| [io.quarkus:quarkus-bom](https://github.com/quarkusio/quarkus) | import | minor | `3.16.3` -> `3.17.0` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | minor | `3.16.3` -> `3.17.0` |
| [org.jboss.logmanager:jboss-logmanager](https://jboss.org) ([source](https://github.com/jboss-logging/jboss-logmanager)) | optional | minor | `3.0.6.Final` -> `3.1.0.Final` |

---

### Release Notes

<details>
<summary>quarkusio/quarkus</summary>

### [`v3.17.0`](quarkusio/quarkus@3.16.4...3.17.0)

[Compare Source](quarkusio/quarkus@3.16.4...3.17.0)

### [`v3.16.4`](https://github.com/quarkusio/quarkus/releases/tag/3.16.4)

[Compare Source](quarkusio/quarkus@3.16.3...3.16.4)

##### Complete changelog

-   [#&#8203;37040](quarkusio/quarkus#37040) - The flyway extension generates Kubernetes resources as if quarkus.flyway.enabled was a runtime property
-   [#&#8203;42446](quarkusio/quarkus#42446) - Add explanation/concept for extension maturity model
-   [#&#8203;44367](quarkusio/quarkus#44367) - Gradle 3.16 fails with missing required property `additionalForcedProperties`
-   [#&#8203;44399](quarkusio/quarkus#44399) - Declaring explicitly the build service in the QuarkusBuildTask
-   [#&#8203;44433](quarkusio/quarkus#44433) - Reflection free serializers ArrayIndexOutOfBoundsException
-   [#&#8203;44438](quarkusio/quarkus#44438) - Gradle `buildForkOptions` no longer used since quarkus 3.16.1
-   [#&#8203;44457](quarkusio/quarkus#44457) - Support for short and uncommon field names like set, get, and is
-   [#&#8203;44468](quarkusio/quarkus#44468) - Use `QUARKUS_FLYWAY_ACTIVE` instead of `QUARKUS_FLYWAY_ENABLED` env in Kubernetes resources
-   [#&#8203;44472](quarkusio/quarkus#44472) - Kotlin native Jackson serialization regression: EmptyList & EmptyMap missing
-   [#&#8203;44480](quarkusio/quarkus#44480) - Fix nullpointer on null code websockets-next
-   [#&#8203;44493](quarkusio/quarkus#44493) - Using BuildForkOptions in QuarkusBuildTask
-   [#&#8203;44494](quarkusio/quarkus#44494) - Register Kotlin's empty list and map for reflection
-   [#&#8203;44505](quarkusio/quarkus#44505) - Log in smallrye-jwt and oauth2 extensions when no bearer access token is available
-   [#&#8203;44507](quarkusio/quarkus#44507) - Fixed Timestamp not being set for otel log signals
-   [#&#8203;44509](quarkusio/quarkus#44509) - Updates to Infinispan 15.0.11.Final
-   [#&#8203;44515](quarkusio/quarkus#44515) - Coordinated Vert.x 4.5.11 upgrades
-   [#&#8203;44531](quarkusio/quarkus#44531) - Correct image file name to resolve broken image
-   [#&#8203;44537](quarkusio/quarkus#44537) - Update smallrye-jwt to 4.6.1
-   [#&#8203;44545](quarkusio/quarkus#44545) - Wrong index of ParameterizedType argument of Map when register type to be generated in JacksonCodeGenerator
-   [#&#8203;44571](quarkusio/quarkus#44571) - Update `CacheJsonRPCService.java` reference
-   [#&#8203;44574](quarkusio/quarkus#44574) - Grammar corrections for en-us

</details>

<details>
<summary>jboss-logging/jboss-logmanager</summary>

### [`v3.1.0.Final`](https://github.com/jboss-logging/jboss-logmanager/releases/tag/v3.1.0.Final): 3.1.0.Final

[Compare Source](jboss-logging/jboss-logmanager@3.0.6.Final...v3.1.0.Final)

#### What's Changed

-   \[LOGMGR-345] Ensure logger FQCN is correct for system logger by [@&#8203;dmlloyd](https://github.com/dmlloyd) in jboss-logging/jboss-logmanager#457
-   Migrate tests to keep the log files that were created. Put the log fi… by [@&#8203;jamezp](https://github.com/jamezp) in jboss-logging/jboss-logmanager#459
-   Bump org.junit:junit-bom from 5.10.1 to 5.10.2 by [@&#8203;dependabot](https://github.com/dependabot) in jboss-logging/jboss-logmanager#461
-   \[LOGMGR-346] Bump org.jboss.modules:jboss-modules from 2.1.2.Final to 2.1.3.Final by [@&#8203;dependabot](https://github.com/dependabot) in jboss-logging/jboss-logmanager#462
-   \[LOGMGR-347] Do not use deprecated SmallRye Common OS `Process` by [@&#8203;dmlloyd](https://github.com/dmlloyd) in jboss-logging/jboss-logmanager#464
-   \[LOGMGR-349] Bump org.eclipse.parsson:parsson from 1.1.5 to 1.1.6 by [@&#8203;dependabot](https://github.com/dependabot) in jboss-logging/jboss-logmanager#466
-   \[LOGMGR-351] Fix periodic file rotation by week, month, year. by [@&#8203;alex-pumpkin](https://github.com/alex-pumpkin) in jboss-logging/jboss-logmanager#468
-   Bump org.jboss.modules:jboss-modules from 2.1.3.Final to 2.1.5.Final by [@&#8203;dependabot](https://github.com/dependabot) in jboss-logging/jboss-logmanager#467
-   \[LOGMGR-350] Avoid TCCL when configuring the log manager by [@&#8203;dmlloyd](https://github.com/dmlloyd) in jboss-logging/jboss-logmanager#469
-   \[LOGMGR-351] Remove the deprecated per-deployment logging options. by [@&#8203;jamezp](https://github.com/jamezp) in jboss-logging/jboss-logmanager#471
-   Bump org.junit:junit-bom from 5.10.2 to 5.10.3 by [@&#8203;dependabot](https://github.com/dependabot) in jboss-logging/jboss-logmanager#478
-   Bump org.jboss.byteman:byteman-bmunit5 from 4.0.22 to 4.0.23 by [@&#8203;dependabot](https://github.com/dependabot) in jboss-logging/jboss-logmanager#476
-   Bump org.junit:junit-bom from 5.10.3 to 5.11.2 by [@&#8203;dependabot](https://github.com/dependabot) in jboss-logging/jboss-logmanager#488
-   Bump org.junit:junit-bom from 5.11.2 to 5.11.3 by [@&#8203;dependabot](https://github.com/dependabot) in jboss-logging/jboss-logmanager#490
-   \[LOGMGR-354] Avoid expensive JLine setup on JDK 23+ by [@&#8203;dmlloyd](https://github.com/dmlloyd) in jboss-logging/jboss-logmanager#491
-   Save head encoding on sanitized String(s) by [@&#8203;franz1981](https://github.com/franz1981) in jboss-logging/jboss-logmanager#492
-   Use `NO_FORMAT` when using parameterless log methods by [@&#8203;dmlloyd](https://github.com/dmlloyd) in jboss-logging/jboss-logmanager#493
-   Switch to formal module descriptor by [@&#8203;dmlloyd](https://github.com/dmlloyd) in jboss-logging/jboss-logmanager#494
-   Module descriptor updates by [@&#8203;dmlloyd](https://github.com/dmlloyd) in jboss-logging/jboss-logmanager#496
-   Bump org.jboss.modules:jboss-modules from 2.1.5.Final to 2.1.6.Final by [@&#8203;dependabot](https://github.com/dependabot) in jboss-logging/jboss-logmanager#495
-   Add smart service provider method by [@&#8203;dmlloyd](https://github.com/dmlloyd) in jboss-logging/jboss-logmanager#497

#### New Contributors

-   [@&#8203;alex-pumpkin](https://github.com/alex-pumpkin) made their first contribution in jboss-logging/jboss-logmanager#468
-   [@&#8203;franz1981](https://github.com/franz1981) made their first contribution in jboss-logging/jboss-logmanager#492

**Full Changelog**: jboss-logging/jboss-logmanager@3.0.4.Final...v3.1.0.Final

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
dmlloyd added a commit to dmlloyd/jboss-logmanager that referenced this pull request Dec 4, 2024
@dmlloyd
Fix header encoding regression
294cdc8 
Problem was introduced by jboss-logging#492.
@dmlloyd dmlloyd mentioned this pull request Dec 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmlloyd dmlloyd dmlloyd approved these changes

@jamezp jamezp jamezp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@franz1981 @dmlloyd @jamezp