Skip to content

Add warnings when JournalStorage lock acquisition is delayed#6361

Merged
not522 merged 5 commits intooptuna:masterfrom
sawa3030:add-warning-for-journal-lock-1
Dec 12, 2025
Merged

Add warnings when JournalStorage lock acquisition is delayed#6361
not522 merged 5 commits intooptuna:masterfrom
sawa3030:add-warning-for-journal-lock-1

Conversation

@sawa3030
Copy link
Copy Markdown
Collaborator

@sawa3030 sawa3030 commented Nov 28, 2025

Motivation

  • This PR is a follow-up to Prevent Lock Blocking by Adding Timeout to JournalStorage #5971.
  • Currently, when JournalStorage keeps retrying to acquire a file lock, there is no feedback to the user. This can make it hard to tell whether the process is still running or stuck.
  • This PR adds warnings when lock acquisition takes a long time, so users can see that the process is alive and waiting for the lock.

Description of the changes

  • Add a warning in the acquire method of JournalFileSymlinkLock and JournalFileOpenLock that is emitted when acquiring the file lock takes longer than a certain threshold.

Note

  • warning_interval is tentatively set to 10 seconds, and this value is open for discussion.

@c-bata
Copy link
Copy Markdown
Member

c-bata commented Nov 28, 2025

@not522 @gen740 Could you review this PR?

Comment on lines +183 to +188
if time.monotonic() - last_warning_time > warning_interval:
optuna_warn(
f"It is taking longer than {warning_interval} seconds to acquire "
f"the lock file: {self._lock_file} Retrying..."
)
last_warning_time = time.monotonic()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about issuing a warning even when self.grace_period is None?

Suggested change
if time.monotonic() - last_warning_time > warning_interval:
optuna_warn(
f"It is taking longer than {warning_interval} seconds to acquire "
f"the lock file: {self._lock_file} Retrying..."
)
last_warning_time = time.monotonic()
if time.monotonic() - last_warning_time > warning_interval:
optuna_warn(
f"It is taking longer than {warning_interval} seconds to acquire "
f"the lock file: {self._lock_file} Retrying..."
)
last_warning_time = time.monotonic()

@sawa3030
Copy link
Copy Markdown
Collaborator Author

sawa3030 commented Dec 4, 2025

Thank you for the suggestion. I've updated the code accordingly. PTAL!

Copy link
Copy Markdown
Member

@gen740 gen740 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gen740 gen740 removed their assignment Dec 5, 2025
@not522
Copy link
Copy Markdown
Member

not522 commented Dec 8, 2025

How about issuing a warning right after if err.errno == errno.EEXIST:? With the current implementation, it might get skipped by continue before issuing the warning.

@sawa3030
Copy link
Copy Markdown
Collaborator Author

Thank you for the suggestion. I’ve applied this change!

@not522 not522 changed the title Add warnings when JournalStorage lock acquisition is delayed Add warnings when JournalStorage lock acquisition is delayed Dec 12, 2025
@not522 not522 added the enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. label Dec 12, 2025
@not522 not522 added this to the v4.7.0 milestone Dec 12, 2025
Copy link
Copy Markdown
Member

@not522 not522 left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM!

@not522 not522 merged commit 0afbe9f into optuna:master Dec 12, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Change that does not break compatibility and not affect public interfaces, but improves performance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants